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

ISSUE-177: LoD Autocomplete Fixes and Improvements #178

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Conversation

DiegoPino
Copy link
Member

@DiegoPino DiegoPino commented Jun 18, 2024

See #177
@livsolis this is WIP. Will keep updating this list as we progress

  • First pass of fixes @alliomeria. I tested with LoD From CSV and then cloned code. So ... someone (me too?) needs to test with the other ones. I feel confident about my code but would not hurt to double check all (or some random) LoD autocomplete elements for functionality when limited to 1. But honestly, up to you how we test this.
  • Second pass. Contains v/s Starts with are respected. Also Limit and also how many characters (spaces are trimmed) kick off autocomplete
  • Third pass. You can use also now extra columns (max 2) for additional context
  • Fourth Pass. (old bug!) We don't allow Drupal to cache the autocompletes during interactions which would lead in the past, after a selection was made, to have on the same autocomplete next time the URL coming up instead of the label.

@alliomeria I tested with LoD From CSV and then cloned code. So ... someone (me too?) needs to test with the other ones. I feel confident about my code but would not hurt. Up to you how we test this.
@DiegoPino DiegoPino self-assigned this Jun 18, 2024
@DiegoPino DiegoPino added bug Something isn't working enhancement New feature or request UI/UX The thing people do when in front of a screen Release Duties Things you do to keep Development Flowing and Users happy Webform Elements Things with input to fill and interact with while ingesting metadata LoD and Auth Controller A Tree of Apples labels Jun 18, 2024
@DiegoPino DiegoPino added this to the 1.4.0 milestone Jun 18, 2024
@alliomeria
Copy link
Contributor

How about I test this in my soon-to-be-born new 1.4.0 local deployment, set all the default multiple values webform LoD elements to single value, test drive there? is that too late-in-the-game for this?

@DiegoPino
Copy link
Member Author

@alliomeria ok cool. Let me work now until I fade into dust fixing the "match" operator + the extended description thing and I let you know Thursday first hour when done so you can test?

@alliomeria
Copy link
Contributor

Please don't fade into dust! Sounds good, thanks @DiegoPino !

…ete limit

I just wonder here if we need to sort on the "contains" on the position of appearance of the query @alliomeria ? Right now i am listing back the autocomplete list based on the actual "ocurrence" in the ROWS. So row 2 in the CSV appears first than row 10... (kinda obvious)
@DiegoPino
Copy link
Member Author

@livsolis @alliomeria I need a clarification. For the extended description from the CSV. Do you also need/want to have "matches" on them? As an option? We tried that in the past and honestly (with Getty) it brings tons of false positives obscuring the actual Labels that match many times. But let me know if that is a need or not

@alliomeria
Copy link
Contributor

alliomeria commented Jun 18, 2024

I would definitely recommend -not- searching within the supplementary text for matches @DiegoPino. I think that was a leading reason for the extra-extra fuzziness and false positives.

@livsolis
Copy link

I agree with Allison. At least for some use cases I'm thinking of, the textual string might be very different from the term (e.g. a bio of a name)

Basically no longer caching for autocompletes which means URLs don't will override Labels.
@DiegoPino
Copy link
Member Author

@livsolis @alliomeria this covers the basic needs for now

  • All bugs fixed
  • Additional columns for context

Have a good weekend and stay cool

@DiegoPino
Copy link
Member Author

Hi @livsolis @alliomeria. I will merge this

This request/use case:

  • Use case: In addition to the URL/URI, I would like to store "type" (personal, corporate, family) for names will have to wait until after the official release of 1.4.0. But will be worked on. So I will keep the issue open and mark as "partially resolved"

@DiegoPino DiegoPino merged commit 1affd7b into 1.4.0 Jun 24, 2024
@alliomeria
Copy link
Contributor

Sounds great, thank you for all your work on this @DiegoPino !

@livsolis
Copy link

@alliomeria @DiegoPino Let me know if you want me to test anything somewhere!

@alliomeria
Copy link
Contributor

alliomeria commented Jun 24, 2024

Hello @livsolis! Thanks for offering to test. In your local instance, if you can test out this ISSUE-177 that would be great.

Starting from inside your archipelago-deployment folder, run each command separately:
cd archipelago-deployment/web/modules/contrib/webform_strawberryfield
git fetch
git switch ISSUE-177
git pull
docker exec -ti esmero-php bash -c "drush updatedb;drush cr"

Let us know if you run into any issues or have questions. Thanks again!

@livsolis
Copy link

Hey @alliomeria our webmaster Paul ( @wpwentzell ) tried your command but didn't see the option to switch to that issue. Are you sure about the command? We're running 1.3.0.
image

@DiegoPino
Copy link
Member Author

@livsolis you might need to do a
git fetch before running those commands to refresh the available branches first. Since ISSUE-177 might not be present in your local cache still

@livsolis
Copy link

OK, we're in! Starts With vs. Contains is starting strong 💃 🪩🕺! I'll continue testing other parts of the issue. :)
Screenshot 2024-06-24 at 2 28 13 PM

@livsolis
Copy link

livsolis commented Jul 2, 2024

I see context! Gracias!
Screenshot 2024-07-02 at 8 39 12 AM

@alliomeria
Copy link
Contributor

Great! Thanks for your feedback @livsolis !

@livsolis
Copy link

livsolis commented Jul 2, 2024

Ok, autosuggest/autocomplete and other functionality is back when "Limited" to 1 is selected in Allowed number of of values in the element options. One thing a little bonus to maybe address in the rest of the fix for this issue... When the allowed values is set to 1, the functionality is there, but the gray box that groups the two fields is missing. From a UX perspective, the gray box visually groups the two fields so you know they are a set and you don't need to edit the field with the URI.
In the screenshot below, the Fixes test element is set to Limited/2, matching the style of the Unlimited setting for Places test 3:
Screenshot 2024-07-02 at 8 50 13 AM
In contrast, the screenshot below represents the field when the field is limited to 1 value:
Screenshot 2024-07-02 at 8 57 44 AM

I think grouping the elements in the gray box would be helpful, even if the +/- box button is unnecessary so the fields are linked. This is minor/aesthetic, but just something to improve on IMO. Also note the larger field set text in the second image, which seems preferable (e.g. "Fixes test")

@DiegoPino
Copy link
Member Author

@livsolis for the grey box, you can define the wrapper (fieldset) used by multiple values but also by single values (and css classes, spacing, etc) to match if you need a consistent UI. Give https://www.drupal.org/docs/8/modules/webform/webform-cookbook/how-to-alter-properties-of-a-composites-sub-elements this a read but also experiment with looks/css classes etc.

Because styling is a theme decision we can't make that change/unify at code level. There might be folks that prefer to have visual differentiation for this too. One never knows.

@livsolis
Copy link

livsolis commented Jul 2, 2024

OK, we can play around with the styles. Thanks for the link.

@livsolis
Copy link

Hey @DiegoPino @alliomeria , I have been testing bios (contexutal info in parens) with names, our craziest taxonomy. I think it is extremely useful and will guide people to the right name. So many. many thanks again! One formatting question. The bios do make it harder to read the term you want to select. I think formatting could help. e.g. either making the bios text gray or making the label bold. Is that something easily doable either on our end or yours? This is obvs. the enhanciest of enhancements. Also, I think I'm going to cut off bios at ~200 characters, but even then, I think I'd still have legibility issues.

Screenshot 2024-07-11 at 10 27 43 AM

@DiegoPino DiegoPino deleted the ISSUE-177 branch October 1, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request LoD and Auth Controller A Tree of Apples Release Duties Things you do to keep Development Flowing and Users happy UI/UX The thing people do when in front of a screen Webform Elements Things with input to fill and interact with while ingesting metadata
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants