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 support for inline, stack label in autocomplete #1994

Merged
merged 24 commits into from Mar 24, 2022

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Mar 23, 2022

Fixes: https://github.com/github/accessibility/issues/897, https://github.com/github/accessibility/issues/924
Design reference

What are you trying to accomplish?

We're making changes to the autocomplete API which includes changes to the markup and styles.
In order to support the styles recommended by @alliethu in Design reference, we must make some CSS changes.

We want to support:

We should make sure:

  • The autocomplete results are aligned to the input field.
  • The icon positioning looks ok.
  • Accessible examples

This introduces the following new classes:

  • autocomplete-body to wrap the input field and results for correct positioning
  • autocomplete-label-stacked to support stacked label position
  • autocomplete-label-inline to support inline label position (which switches to stacking on smaller viewport)
  • autocomplete-embedded-icon to support an embedded icon in the input field

What should reviewers focus on?

When this is ready for review, let's get @alliethu eyes on the Preview.

Are additional changes needed?

  • No, this PR should be ok to ship as is. 🚢

@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2022

🦋 Changeset detected

Latest commit: 94cd787

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

This PR includes changesets to release 1 package
Name Type
@primer/css Minor

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

@lindseywild
Copy link
Contributor

Looking at the preview link, it doesn't seem like the "Default" label is stacked, are you seeing the same thing?

screenshot of the default example showing that the label in inline when it should be stacked

@khiga8
Copy link
Contributor Author

khiga8 commented Mar 23, 2022

Ah @lindseywild sorry this is actively WIP and being committed on! Let me ping again when it's actually ready

@smockle
Copy link
Member

smockle commented Mar 23, 2022

From @khiga in #1994 (comment):

  • TODO: Embedded icon with visible label
  • TODO: Embedded icon with hidden label

[…]

  • The icon positioning looks ok.

Do these items imply this PR will also fix https://github.com/github/accessibility/issues/924?

@khiga8
Copy link
Contributor Author

khiga8 commented Mar 23, 2022

The TODO is up for grabs. I just pushed up a commit with examples on the docs page to validate the TODO work, but feel free to change those.

As you make changes you can run

npm run dist

to recompile CSS and run

npm run dev

to run the doc site (http://localhost:8000/components/autocomplete)

@khiga8
Copy link
Contributor Author

khiga8 commented Mar 23, 2022

@smockle yes! let me update this issue description.

@lindseywild
Copy link
Contributor

lindseywild commented Mar 24, 2022

Update: it looks like the media query isn't being applied to the example (which makes sense), but all of the code is accurate. What do you think about removing the example for smaller screens and just make a note that it stacks on mobile?

@khiga8 just a heads up, it looks like there is some jankiness in Firefox with the position: absolute on the results. I can take a look after addressing the icon styling, unless you'd prefer to look sooner!

screenshot of the autocomplete element in firefox where the results container is overlapping the input element

@khiga8
Copy link
Contributor Author

khiga8 commented Mar 24, 2022

Hi @langermank, this is ready for another review!

Copy link
Contributor

@langermank langermank 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 for making those changes! I marked this as minor because I don't think any of these are breaking changes. I tested out the changes to .autocomplete-results in the current primer docs and it seems okay 🤞

@khiga8
Copy link
Contributor Author

khiga8 commented Mar 24, 2022

Thank you @langermank!

Could we get a Primer CSS version bump after this gets merged so we can continue our work on the Autocomplete PVC component? 🙏

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.

None yet

5 participants