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

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

Merged
merged 24 commits into from
Sep 10, 2024

Conversation

jamieshark
Copy link
Contributor

@jamieshark jamieshark commented Aug 30, 2024

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 Screenshot of error banner Screenshot of no results

Integration

List the issues that this change affects.

Risk Assessment

  • 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)
  • Tested in Chrome
  • Tested in Firefox
  • 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.

@jamieshark jamieshark requested review from a team as code owners August 30, 2024 18:37
@jamieshark jamieshark requested review from langermank and removed request for a team August 30, 2024 18:37
Copy link

changeset-bot bot commented Aug 30, 2024

🦋 Changeset detected

Latest commit: 9eb76f1

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

Copy link
Contributor

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.

1 similar comment
Copy link
Contributor

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.

Copy link
Contributor

github-actions bot commented Aug 30, 2024

⚠️ 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

@github-actions github-actions bot requested a review from a team as a code owner August 30, 2024 18:53
Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Nice! I'll let @langermank give the final go-ahead tho 😄

app/components/primer/alpha/select_panel.html.erb Outdated Show resolved Hide resolved
app/components/primer/alpha/select_panel.pcss Show resolved Hide resolved
@primer primer deleted a comment from github-actions bot Sep 6, 2024
@primer primer deleted a comment from github-actions bot Sep 6, 2024
@maximedegreve
Copy link
Contributor

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

  1. The margin above the title should only be 8px and not 16px.
  1. 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.
CleanShot.2024-09-09.at.10.22.10.mp4
  1. 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.

  2. The empty state is also missing a min-height similar to the error/loading states

Screenshot 2024-09-09 at 10 29 25
  1. The playground example changes in width after results are loaded in. This causes a layout swift we want to avoid.
layoutshift.mp4

@primer primer deleted a comment from github-actions bot Sep 9, 2024
@jamieshark
Copy link
Contributor Author

jamieshark commented Sep 9, 2024

@maximedegreve

  1. 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.
  2. I have created another issue for this: https://github.com/github/primer/issues/3932
  3. I have created another issue for this: https://github.com/github/primer/issues/3933
  4. I have created an issue for this (https://github.com/github/primer/issues/3934) and have addressed this in my recent changes.
  5. 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.

@jamieshark jamieshark changed the title [SelectPanel] Filter input and loading/error panel fixes [SelectPanel] Filter input border and loading/error panel min-height Sep 9, 2024
Copy link
Contributor

@maximedegreve maximedegreve left a comment

Choose a reason for hiding this comment

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

Approving since the other reported issues will be fixed as separate issues.

@kendallgassner
Copy link
Contributor

✅ 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
Copy link
Contributor

🔴 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.

Copy link
Contributor

@kendallgassner kendallgassner left a comment

Choose a reason for hiding this comment

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

💟 ✅

Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Nice!

@jamieshark jamieshark merged commit b57b63a into main Sep 10, 2024
37 checks passed
@jamieshark jamieshark deleted the jshark/select-panel-design branch September 10, 2024 17:58
@primer primer bot mentioned this pull request Sep 10, 2024
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.

5 participants