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

Add affordance to icon selector in docs and fix selection state #6877

Merged
merged 5 commits into from
Jun 28, 2024

Conversation

evansjohnson
Copy link
Contributor

@evansjohnson evansjohnson commented Jun 27, 2024

Fixes no issue

Checklist

  • [n/a] Includes tests
  • [n/a] Update documentation

Changes proposed in this pull request:

  • Add better affordance that something needs to be typed for icons to show. I thought the docs were broken and didn't initially try typing anything here.
  • Fix selection/active state. f0a9b09#diff-8194812817b7f429b5320bb9be65c267dea2308f9a5b914411859fc8d61d18b0R222 seems to be a breaking change (see how some other docs examples were updated). Maybe this was only part of the initial BP 5 release, but didn't seem to be mentioned in the upgrade guide. In any case this has been out long enough that it should be considered expected behavior at this point
  • Update docs to mention selected only applies with the "listoption" role - I had to dig into the code to figure out what was going wrong here and initially thought selection state was entirely broken.

Reviewers should focus on:

  • Anything else related that should be fixed
  • Any unintended breaking changes from this PR

Screenshot

Current:
I found this to be confusing - this looks like it came up in the PR that introduced but it was a big one and I suppose was never addressed
Screenshot 2024-06-27 at 3 01 48 PM
no indication of selection within menu - note selected icon shown in button:
Screenshot 2024-06-27 at 3 01 55 PM

This PR:
custom placeholder text:
Screenshot 2024-06-27 at 3 02 10 PM
selection state:
Screenshot 2024-06-27 at 3 02 15 PM

@evansjohnson evansjohnson changed the title Add affordance to icon selector in docs Add affordance to icon selector in docs and fix selection state Jun 27, 2024
@svc-palantir-github
Copy link

fix selection state

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@evansjohnson
Copy link
Contributor Author

Noticed #6878 while testing but don't want to put more time into this for now

@svc-palantir-github
Copy link

more correct

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@@ -159,6 +167,7 @@ export class Select<T> extends AbstractPureComponent<SelectProps<T>, SelectState
filterable = true,
disabled = false,
inputProps = {},
placeholder = "Filter...",
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 used an ellipsis in Start typing to search… but didn't want to convert all ellipsis in BP to the actual character until a major version bump since this could be a breaking change (tests that rely on finding elements by default text)

*
* @default "Filter..."
*/
placeholder?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

calling out this small new feature. this has some potential to be used inappropriately, but I don't think "Filter..." is always the best indication here particularly since the user is responsible for defining the match predicate and may not strictly implement a filter

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree just "Filter..." may not be that valuable to a user. I also think we'd want this to be customizable for translations. I'm actually surprised a hard-coded English string has survived this long in Blueprint without someone flagging that a translation is necessary.

Thanks for adding!

@@ -77,9 +80,6 @@ export class IconSelect extends React.PureComponent<IconSelectProps> {
};

private filterIconName = (query: string, iconName: IconName | typeof NONE) => {
if (iconName === NONE) {
return true;
}
if (query === "") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this prevents loading the full list on empty query - there's over 600 icons and it's a bit laggy if showing all... and doesn't quite feel worth bringing in a virtualized list for

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 was a bit surprised it was starting to struggle with just 600 items, in case this raises any concerns

gluxon
gluxon previously approved these changes Jun 28, 2024
Copy link
Contributor

@gluxon gluxon left a comment

Choose a reason for hiding this comment

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

Thank you!

* Defining this will set the `aria-selected` attribute and apply a
* "check" or "blank" icon on the item (unless the `icon` prop is set,
* which always takes precedence).
* Whether this item should appear selected - `roleStructure` must be `"listoption"` for this to be
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding this comment!

roleStructure must be "listoption" for this to be applied.

This prop being a no-op depending on roleStructure would have surprised me too, especially since "listoption" isn't the default.

*
* @default "Filter..."
*/
placeholder?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree just "Filter..." may not be that valuable to a user. I also think we'd want this to be customizable for translations. I'm actually surprised a hard-coded English string has survived this long in Blueprint without someone flagging that a translation is necessary.

Thanks for adding!

@policy-bot policy-bot bot dismissed gluxon’s stale review June 28, 2024 18:03

Invalidated by push of 8514c6c

@svc-palantir-github
Copy link

cleanup none

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@evansjohnson evansjohnson merged commit 855a067 into develop Jun 28, 2024
13 checks passed
@evansjohnson evansjohnson deleted the evanj/icon-select branch June 28, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants