Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select / typeahead combo for foreign key field #222

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

mkschell
Copy link
Contributor

Apologies for discontiguous PR.

Relevant discussion points from previous (#210

zachdaniel:

So, this is very cool, but I do worry that it can only really be used for low cardinality relationships. i.e if you had 100k users, you would need a type-ahead that would do things like fetch data after a few characters were displayed etc.

If we add this and someone misuses it, it might look good in dev but then when running in prod it could cause significant issues.

Would you be open to making it a typeahead? It doesn't have to look beautiful :)

netProphET:

Yeah that concern crossed my mind as well, and a typeahead could be a good solution. Definitely don't want to provide the rope to hang a production site with. However, not sure I love typeahead for the really low cardinality relationships, e.g. say you have 3 Categories to pick from. I'm happy to play around with this till we have something that will scale.

zachdaniel:

I think it would make sense to do one of two things:
fetch some limited number of results, and if there are less than that that come back, we show a select, and if there are more we show a typeahead.
make it configurable from the outset in the source form. Like configuring it from the source.

So I learned a lot doing this, which is great. I may have gone overboard in how I implemented some of this due to my inexperience with some of the idioms. I'm also unsure about the naming choices for the DSL: admin.label_field and admin.relationship_select_max_items. Also I did not implement a minimum # of characters to wait for in the typeahead yet. It doesn't feel too terrible to me to have it start showing results after 1 keystroke (it does use a 300 ms debounce).

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

config/config.exs Outdated Show resolved Hide resolved
@mkschell
Copy link
Contributor Author

mkschell commented Dec 2, 2024

Quick update on this: work is still in progress. I've been working with Ash Admin's demo app, which surfaces some concepts I'm still absorbing, and has exposed some weaknesses in my implementation here. More soon.

@mkschell
Copy link
Contributor Author

mkschell commented Feb 4, 2025

Couple of vids showing this in action. Not shown is a has_many, but I could do one from a different project if desired.

AshAdmin_CreateTicket_ManageRelationships.mp4
AshAdmin_UpdateTicket_BelongsTo.mp4

@mkschell mkschell marked this pull request as ready for review February 4, 2025 19:33
@mkschell
Copy link
Contributor Author

mkschell commented Feb 4, 2025

There's only one failing Dialyzer check that I think might be correctable in this PR, but I'm not sure how. It works for me. It might be a language version thing?

AshAdmin.Components.Resource.RelationshipField, line 163

            >
              <% suggestion_name =
                String.replace(suggestion_name, ~r/(#{@search_term})/i, "<b>\\0</b>", [
                  :case_insensitive
                ]) %>

@mkschell
Copy link
Contributor Author

mkschell commented Feb 4, 2025

Oh, it's the raw user generated data in the regex. One more commit coming right up.

@zachdaniel
Copy link
Contributor

Really exciting stuff! This has a dialyzer error still. Could you show what it looks like for a has_many case? We can add more relationships to the dev example app so there is something there to demo.

@@ -37,6 +37,8 @@ Configure the admin dashboard for a given resource.
| [`resource_group`](#admin-resource_group){: #admin-resource_group } | `atom` | | The group in the top resource dropdown that the resource appears in. |
| [`show_sensitive_fields`](#admin-show_sensitive_fields){: #admin-show_sensitive_fields } | `list(atom)` | | The list of fields that should not be redacted in the admin UI even if they are marked as sensitive. |
| [`show_calculations`](#admin-show_calculations){: #admin-show_calculations } | `list(atom)` | | A list of calculation that can be calculate when this resource is shown. By default, all calculations are included. |
| [`label_field`](#admin-label_field){: #admin-label_field } | `atom` | | The field to use as the label when the resource appears in a relationship select or typeahead field on another resource's form. |
| [`relationship_select_max_items`](#admin-relationship_select_max_items){: #admin-relationship_select_max_items } | `integer` | | The maximum number of items to show in a select field when this resource is shown as a relationship on another resource's form. If the number of related resources is higher, a typeahead selector will be used. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a reasonable default value, like 50-100 I think.


defp calculation(label_field, AshPostgres.DataLayer) do
expr(
fragment(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use contains(^ref(label_field), ^search_term) instead of this I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, I'd like to add a starts_with expression to core to make this work 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calculation is used to rank results based on the position of the search term in the matched result. I kinda thought it made for neat UI, in case all the user can do is guess at keywords in a large data set, but maybe that's overkill.

If I'm understanding your suggestion correctly, and if you do prefer returning only results that start with the user supplied string, then I can go ahead and make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't mind contributing starts_with as you suggested, regardless of the decision for ash_admin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right right, we should add string_position instead of starts_with so you can rank them the way you have here 😄

@zachdaniel
Copy link
Contributor

This looks great! Small comments, would you be interested in contributing starts_with to ash and ash_sql to support this?

@zachdaniel
Copy link
Contributor

Fixed dialyzer in main, will be releasing the ash packages today or tomorrow, so you could then update this to use the new str_pos function 😄

@mkschell
Copy link
Contributor Author

@zachdaniel Sweet. Regarding dependencies, once this happens, would you prefer I bump the Ash version that ash_admin depends on, or put code in place to check for the string_position function and fall back to something else? I already have the latter in place, but if you like having the libraries require the latest core, I'm happy to do that as it's a bit cleaner.

@zachdaniel
Copy link
Contributor

We can bump the ash version req. Too complex to play that game when there isn't really a good reason folks can't upgrade.

@zachdaniel
Copy link
Contributor

@mkschell everything you need should be released now 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants