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

StringArrayEditor revision and update #3469

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

Gazook89
Copy link
Collaborator

@Gazook89 Gazook89 commented May 10, 2024

This is basically an implementation of what I wrote in #3466. It restructures the DOM of the StringArrayEditor and adds more functionality, such as better keyboard interaction. It removes what I consider "extra buttons" which I don't see used in many other tag editors/inputs. The styling is yet unfinished, but even at this stage is more inline with existing HB styling.

image

Keyboard

context action result
new text in new-tag-input, or editing existing tag , | Enter submits text as tag and focus new-tag-input
focus within tag-container (unless in input) ArrowLeft | ArrowRight moves focus left/right through tags and new-tag-input, within tagContainer
focus on existing tag Enter | Space open tag and focus input for editing
focus within input in tag or new-tag-input Escape blur/exit input without submitting, move focus to tag
focus on existing tag Delete delete existing tag
focus on element prior to tag-container Tab | Shift + Tab move focus to new-tag-input, skipping existing tags (instead, use arrow keys to navigate thru individual tags)

Things I want to hammer out:

  • fix: need to take datalist options as a prop-- currently its static and shows same list for tags and authors.
  • fix: if an input inside an existing tag is cleared out (everything is deleted) and then you move to the next tag, by pressing Enter or ,, the original text is reset and the tag isn't deleted.
  • fix: if using Delete to delete characters after text cursor, the whole tag is deleted.
  • fix: prevent 'resetting' of input text when you hit Space while editing an existing tag.
  • do not use .invalid styling when input field is empty, but still prevent entry of empty tags
  • allow entry of tag schemes (meta:, systems:, etc) using either upper or lowercase, but convert to lowercase when submitted.
  • if using backspace in existing tag and no text remains in input, it throws a non-breaking error in console...should prevent that.
  • fix: only render datalist if datalist prop exists
  • factor in the issue mentioned in draft PR Tag trim #3284
  • fix: the cleanValue() method on stringArrayEditor is too specific to one instance of the component (brew tag editor), and has no application for other instances (like 'authors' input). Need to figure out how to pass that type of 'value manipulation' in as a prop? The crux is that the current 'validator' is lenient on the tag scheme case-- it can be entered lowercase or uppercase-- but we actually only accept lowercase here, and cleanValue() was used to convert it to lowercase on submission. So maybe a validator prop and a acceptedValue prop?
  • check accessibility (semantic html, aria roles)

Things I might want to do in this PR, but maybe not:

  • convert to functional component
  • preserve width of tag when switching between "editing" and "not editing"

Things I might do if this PR is merged:

  • rework the css for metadataEditor, to reduce duplicate or unused styles. Possibly separate out the styles for each component (or at least some of them, and utilize css variables to control in metadataEditor.less)

Related Issues:

 

Relevant PR:

#3469

@Gazook89 Gazook89 changed the title add , as a "submit and move to next" key StringArrayEditor revision and update May 10, 2024
@Gazook89 Gazook89 marked this pull request as ready for review May 11, 2024 03:50
@Gazook89 Gazook89 mentioned this pull request May 13, 2024
6 tasks
This allows a custom modification to the values for each instance of the component, passed in via props.
@Gazook89 Gazook89 added UI/UX User Interface, user experience 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure labels May 13, 2024
Copy link
Member

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

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

I like where this is going, but mainly the huge datalist seems a little unwieldy. Not sure how much control we have but I think we can make some tweaks.

} else {
this.addValue(event.target.value);
this.addValue(event.target.value.trim());
Copy link
Member

Choose a reason for hiding this comment

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

modifySubmission already trims the input. Would it make sense to move modifySubmission earlier in the process somehow (maybe at the start of this function?) so we can process all the "input fixes" first and then the other logic later? Otherwise we are repeatedly trimming the input multiple times, scattered across several places. I.e., try to collect any transforms to the user input into a common pipeline.

client/homebrew/editor/metadataEditor/tagDatalist.json Outdated Show resolved Hide resolved

},

renderDatalist : function(){
Copy link
Member

Choose a reason for hiding this comment

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

Concerning the datalist:

  • The list is enormous. Is there any way to trim it down initially with just the major categories or something, and then transition to the full list once the user starts typing?
  • For some reason, I initially had the impression that these were the only allowed options (maybe because the huge list looks so large it might be exhaustive? Maybe I am trained that the little "dropdown" arrow on the textbox means I have to choose one from the list?). Clearly that is not the case because we also allow custom tags, but I wonder if there is some way to better imply that these are only suggestions.
    • Better placeholder text?
    • Remove the dropdown arrow thing?
    • Somehow label the datalist with a title saying "common/suggested tags"
    • Curate or reorder the pre-selected options?
  • On the other hand, the "tag-scheme"s are restricted. Seems like there should be some clear way to convey to the user "here is a list of allowed tag-schemes, and under those tag schemes, here are some recommended tags, but you can also make your own tag using one of these tag-schemes (or even none at all)."
  • If you select type: (or any of the "tag-scheme"-only options, I would expect the datalist to now immediately show me the sub-categories of type:..., but instead I just get a red "invalid" box. It's not until I type some more that the rest of the suggestions appear.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done a great deal of one-on-one usability testing over the last decade, including specifically with introducing datalist auto-completes to users. Many users are already familiar with type-ahead auto-complete from many environments (though not all users, of course). One general principle I found over and over is that users bring expectations of UI behaviour from other sites with them .. which means deviating from standard behaviour (no matter how well intended) can result in more confusion, frustration, and undermining of confidence.

I did find that a long initial droplist encourages free-form typing, which then triggers filtering of the droplist. Whereas a short droplist tends to result in selecting one of the predefined options, almost to the exclusion of free-form typing.

Please do review the individual options in the dataliast. I tried to ensure that there were no confusingly-similar synonyms, and that each option in each scheme had roughly the same level of abstraction.

Copy link
Member

Choose a reason for hiding this comment

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

One general principle I found over and over is that users bring expectations of UI behaviour from other sites with them

I agree. This is where my comment is coming from. A button with a dropdown arrow that when clicked opens up a long list... My learned UI expectation has trained me to not type and just click an item in the list. I only have a sample 4 so far, but everyone else I showed this to earlier did the same thing, so you are right there is probably some common learned expectation that is not being met. My coworker suggested that along with the dropdown arrow, it might be the long long list which matches the vibe I was getting. So I agree I think it's worth looking into.

Copy link
Member

Choose a reason for hiding this comment

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

I did find that a long initial droplist encourages free-form typing, which then triggers filtering of the droplist. Whereas a short droplist tends to result in selecting one of the predefined options, almost to the exclusion of free-form typing.

Ah... I reread this and realized you meant the opposite that we should keep a long list. It's possible my subconscious trained UI expectations are not from the size of the list, but I can tell something is off, discouraging free-form typing.

Maybe its not the size of the whole list but the type/system/meta categories? When it shows "type" followed by 10 entries, and "system" followed by 5 entries, is it subconsciously signalling that there are only 10 types and 5 systems? I.e. if you have found that small lists discourage typing maybe many small lists in sequence compounds this?

Edit: just had another thought. When a list appears beneath the box, it signals to me the familiar "type and filter" behavior. But in this PR, clicking an empty input field, Chrome displays the list separate and to the right, which to me means "pick from this menu". Maybe that's where Im getting the "this list is too long" vibe?

onChange={(e)=>this.setState({ updateValue: e.target.value })}
onBlur={()=>this.closeEditInput(i)}
list={this.props.options?.length > 0 ? `${this.props.id}__tags-precoordinated` : ''}/>
{this.renderDatalist()}
Copy link
Member

Choose a reason for hiding this comment

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

The datalist is already rendered below under the field renderer, and this adds a separate copy for each tag. Does it need to be duplicated for each tag like this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, we only need one <datalist> element. Elsewhere (another PR?) the matter of separating <datalist> as it's own component function has been discussed. (It could also be used for simple <input> elements too, not just StringArrayEditors. We don't have a specific need for that yet.)

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure client/components/combobox.jsx that we use for the Languages selector is already this.

const valueElements = Object.values(this.state.valueContext).map((context, i)=>{
return <React.Fragment key={i}>
<div aria-label={`tag ${context.value}`} className={`tag ${context.editing ? 'editable' : ''}`} tabIndex={-1} onKeyDown={(e)=>this.handleTagKeyDown(e, i)}>
<span className={`tag-text ${context.editing ? 'hidden' : 'visible'}`} key={i} onClick={()=>this.editValue(i)}>{context.value}</span>
Copy link
Member

Choose a reason for hiding this comment

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

If you click on an existing tag to edit it, you can't start typing right away. You have to click again to enter the text box. Is it possible to focus on the textbox with one click?

this.removeValue(index);
} else if(_.includes(['ArrowLeft'], event.key)){
event.target.previousElementSibling?.focus();
} else if(_.includes(['ArrowRight'], event.key)){
Copy link
Member

Choose a reason for hiding this comment

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

You can arrow-right all the way to the final text entry box, but you can't arrow-left from the text entry box back to the other tags.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you try again? I am able to use the arrow key to move from the final input to the previous tag without issue.

However, you are correct if the final input has any text inside of it-- the final input has to be cleared before you can use arrow keys to move out of it. I'm pretty sure this was a conscious choice I made. If someone wanted to move all the way to the beginning of their text using arrow keys, and held down the arrow key to do so, I wouldn't want to move them to the previous tag because they held down the key too long. If necessary, I could detect if they are holding down the arrow key (rather than rapidly pressing it), and only prevent moving to the previous tag if true. But I just didn't think it necessary at the time.

Copy link
Member

Choose a reason for hiding this comment

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

However, you are correct if the final input has any text inside of it-- the final input has to be cleared before you can use arrow keys to move out of it.

Ah, this is what I am seeing then. I was just playing around moving left and right and when I entered the final input I couldn't get back out.

I agree with your assessment that holding down the left arrow shouldn't fully leave the box. Maybe if you can detect that the cursor is in the first character position when the key is first pressed and only transition left in that case. But that is not a dealbreaker for merging.

{this.props.options.map((option)=>{
return <option value={`${option}`}></option>;
})}
</datalist>;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to display a different value in the datalist options text? Just thinking, it may be nice to show something like meta: ... to indicate that meta: by itself isn't a valid option. Then when you click, it just applies meta: and keeps the datalist open to now show the suggestions to fill out the tag.

Or instead of ..., the label shows some example sub-tags to explain what is meant:

Edit: Seems you can add text to the <option> element to display descriptive text:

<option value="type:">(adventure, class, setting, etc.)</option>

image

Maybe combine this with my earlier comment to show a simplified list like this when the textbox is first empty, but then use the full list after the user starts typing and some options have been filtered out.

Copy link
Member

Choose a reason for hiding this comment

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

It may be worth it to just use our existing combobox element from the language selector so we have full customizability. We ran into issues with the datalist for that too, so the work has already been done.

@Gazook89
Copy link
Collaborator Author

Gazook89 commented May 20, 2024

It seems like most of the issue you have is with the datalist (justifiably). Rather than respond to each comment in the review, I'm just going to respond here.

I pretty much just went with what was outlined in #2335, particularly in terms of what options should be in the datalist but also the tiny bit of discussion about how such a long list should be handled. I agree the list is too long, and that such a long list can imply "choose one of these options" particularly without any prompt.

I like the suggestions above but have to think about them a little more.

In the meantime, I had another idea: what if we just used simple tags that aren't "compounds" of "meta:" + "blah" for the input, instead just using different tag input fields for those categories and then turn them into compound tags after entry...so the UI would look something like this:

image

Is this more intuitive, that there are categories of tags? And from this, each input area could have a datalist specific to that category. In our system, the tags would be stored as we have them now, meta:theme etc, and everywhere else that tags are displayed they would show like meta:theme, but in this particular UI where users are editing their tags it would be more structured.


Also mentioned in the linked Discussion is the tag input component from Mantine.dev which might save some hassle. Another one I was looking at is accessible-components/tag-input, preview here. Unfortunately Radix doesn't have a pre-made tag input nor do they really have a pre-made combobox.


I'll knock out a couple of the easy things you mentioned, push them up here, and by then hopefully have more thoughts on how to do the things asked above (or more ideas).

@calculuschild
Copy link
Member

Is this more intuitive...

I see what you mean but I think the added clutter negates most of the benefit.

@Gazook89
Copy link
Collaborator Author

yeah, and it would be a problem if for some reason more 'special' categories were added...you'd have to keep adding more inputs.

Another idea is to make each tag two inputs-- a dropdown for the tag scheme (meta, type, etc) and then a scheme-specific combobox. The left button throws a dropdown, and the chosen value determines which datalist is presented in the right-hand side combo input.

image

Further, it could still allow typing meta:theme in the right hand side for convenience, and on 'submission' would split the tag scheme portion to the left hand side (if valid). My example above should have a 'blank' option, as well. For regular tags, just type in the right-hand side.

The Author's StringArrayEditor/TagInput would still need to be more basic, so I'm not sure if the proposal above would just be a different component, or a configuration of the same component. But just throwing ideas out.

@ericscheid
Copy link
Collaborator

In the meantime, I had another idea: what if we just used simple tags that aren't "compounds" of "meta:" + "blah" for the input, instead just using different tag input fields for those categories and then turn them into compound tags after entry...so the UI would look something like this:

Additional to the clutter aspect, this creates the risk that a user will enter a tag into the wrong field. Compare the combo field, where typing "ad" filters the droplist to type:Adventure and also system:AD&D in the same droplist.

@calculuschild
Copy link
Member

Another idea is to make each tag two inputs-- a dropdown for the tag scheme (meta, type, etc) and then a scheme-specific combobox. The left button throws a dropdown, and the chosen value determines which datalist is presented in the right-hand side combo input.

This is kind of along the lines of what I was thinking when I wrote "Is there any way to trim it [the datalist] down initially with just the major categories or something, and then transition to the full list once the user starts typing?" I.e. make the datalist a two-step process, first picking the scheme and second filling out the tag name. Though I only pictured it as one single textbox.

I am intrigued by this "dual-tag" approach. I say we give it a shot.

Gazook89 and others added 5 commits May 28, 2024 15:14
Any tag can be turned into the active input and the inputref is dynamically assigned to that input.  So the "new tag" input isn't really that different from a "edit existing tag" input.

Some whitespace and formatting changes as well.  Stepped away from stashed changes for 8 days so this isn't the best commit msg.
Should just be whitespace changes due to linting.
made an error ~two commits ago, trying to tie newTagInput together with existing tag inputs.  Keeping them separate....for now.
@Gazook89
Copy link
Collaborator Author

Gazook89 commented May 30, 2024

Discussed briefly in gitter: going to move forward with working on something like the below. Likely I will use a library like FloatingUI to handle the anchor positioning of menus (skipping CSS Anchor API for now since it is only available in Chrome and I don't want broken menus in firefox). Likely I will be working on improving the Combobox.jsx component first, possibly splitting out the "datalist" portion as it's own thing which is then imported to combobox, and add a few props.

image

Styling isn't locked down at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure UI/UX User Interface, user experience
Projects
Status: Pushing to Finish
Development

Successfully merging this pull request may close these issues.

None yet

3 participants