[SelectPanel] Filter input border and loading/error panel min-height

jamieshark commented 2 months ago

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans. Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

This PR addresses a few design items towards the SelectPanel component (https://github.com/github/primer/issues/3875). Namely, it adds a divider below the filter input (if present), it moves the filter banner error to be above the input, and adds a min-height that corresponds to the overlay size for the loading / error states.

Screenshots

Screenshot of loading state with min height

Screenshot of error state with min height

Integration

List the issues that this change affects.

Risk Assessment

[x] Low risk the change is small, highly observable, and easily rolled back.
[ ] Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
[ ] High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

Fixes axe scan violation - This change fixes an existing axe scan violation.
No new axe scan violation - This change does not introduce any new axe scan violations.
New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

[ ] Added/updated tests
[ ] Added/updated documentation
[ ] Added/updated previews (Lookbook)
[x] Tested in Chrome
[x] Tested in Firefox
[x] Tested in Safari
[ ] Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: 9eb76f1ca5aa906753aafaade84a05d78177e68d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | ----------------------- | ----- | | @primer/view-components | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 2 months ago

Uh oh! @jamieshark, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

github-actions[bot] commented 2 months ago

Uh oh! @jamieshark, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

github-actions[bot] commented 2 months ago

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

jamieshark commented 2 months ago

@langermank - I've added a limit_items param for the playground where you can preview the various sizes with a specified number of items. Here's an example of :medium_portrait size with 2 items showing after load.

http://primer-view-components-preview-3044.eastus.azurecontainer.io/view-components/lookbook/inspect/primer/alpha/select_panel/playground?size=medium_portrait&limit_items=2

maximedegreve commented 1 month ago

Hi @jamieshark

It seems like the error states are not styled according to our specifications.

Note how there is a lack of padding under the warning/error icon. Could we also provide a slot to add a description for the error and loading state?

The font in the error banner is also slightly too big.

jamieshark commented 1 month ago

@maximedegreve - the error message shown in the playground is only the default shown by the element. The consumer can provide custom error_content if they like, so I don't think a separate slot is needed, since the consumer can just provide that directly as the error_content slot. I would recommend creating another issue for the loading-state slot, so we are not preventing the other 3 issues from being closed as part of this work.

With regards to the other changes, I've added a little padding between the icon and text (the spec specifies 14px but in the interest of keeping with the system I've given mt-3 which compiles to 16px) and updated the font size and weight of the filter banner.

Edit: I've created a new issue for the loading state slot here: https://github.com/github/primer/issues/3926

maximedegreve commented 1 month ago

Noticed a few small things. Some might be able to be addressed in this issue others might need opening of new issues.

The margin above the title should only be 8px and not 16px.

The example below closes the panel when I initiate a filter query and press enter. Since the focus is the input and not on an item this is unexpected and incorrect. The cursor is also in the wrong spot after opening it again. I can't remember 100% sure anymore but I think it should wipe the filter value when closing the panel.

https://github.com/user-attachments/assets/35aa8dd1-4ff3-4b7f-9027-ab4c5599da3a

Multi select should have visual checkboxes (not semantically) to visually indicate to users it won't close the panel when making a multiple selection. Single select is correct to use the checkmark but multi select isn't. See the screenshots in our docs.
The empty state is also missing a min-height similar to the error/loading states

The playground example changes in width after results are loaded in. This causes a layout swift we want to avoid.

https://github.com/user-attachments/assets/64c0c83a-0f77-4481-9bb9-b225e13f651a

jamieshark commented 1 month ago

@maximedegreve

I have addressed this with my recent changes, though I would like to note that the spec specified 14px and I'm wondering what can be done to mitigate this confusion in the future.
I have created another issue for this: https://github.com/github/primer/issues/3932
I have created another issue for this: https://github.com/github/primer/issues/3933
I have created an issue for this (https://github.com/github/primer/issues/3934) and have addressed this in my recent changes.
I believe this is happening because the overlay size is set to :auto. If you would like to avoid width changes, it might help to select a different size. Otherwise, there's not really any way for us to know the exact width of the content that's coming in.

kendallgassner commented 1 month ago

✅ For single-select, I tested that if a page limit is added the selected state is updated correctly as Items are loaded/unloaded.

A video showing that the selected item state is retained as I filter using the SelectPanel's input.

kendallgassner commented 1 month ago

🔴 I did find one niche bug. If a selectedItem does not initial show due to a page limit... It is never selected. See video:

A video where I change the page limit to 1. The selected item is not loaded. I then filter by the selectItems name showing it is not selected.

primer / view_components