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

feat: bumps autocomplete element package, updates API #1050

Merged
merged 32 commits into from
Mar 29, 2022

Conversation

lindseywild
Copy link
Contributor

@lindseywild lindseywild commented Mar 7, 2022

Closes https://github.com/github/accessibility/issues/796.

Changes

Package updates

  • Bumps autocomplete element package to 3.1.0.

API updates

  • Adds optional clear button (note: we will need to consult with design on this once we have a component in use that is clearable (addresses https://github.com/github/accessibility-audits/issues/230)
  • Adds screen reader feedback element (addresses https://github.com/github/accessibility-audits/issues/228)
  • Add an argument for a user to determine if they want an inline label (stacked by default)
    • If yes, add class autocomplete-label-inline to the label element
    • If no, add class autocomplete-label-stacked to the label element
  • Wrap the input and results in an element with the class autocomplete-body
  • If user chooses to add an icon, wrap the svg and input in a <div class="form-control autocomplete-embedded-icon-wrap"> (see example here)
  • Restrict icon to "Search" icon per @ichelsea

@lindseywild lindseywild requested review from a team and jonrohan March 7, 2022 16:09
integrity sha512-2N6SP/WOvPPnMm5E0uq21AyCxsJ0d6cBxBZ0yVNpvgeaNgPNI8gR1h54c2Ao4iQtvFce6eWdIezBXLDPMPbDag==
"@github/auto-complete-element@^3.1.0":
version "3.1.0"
resolved "https://registry.yarnpkg.com/@github/auto-complete-element/-/auto-complete-element-3.1.0.tgz#afd5ee64a17b4507c42136cfd790103217009b50"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this right that it should change from npmjs.org to yarnpkg.com? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't have an answer why, but I notice @github/details-menu-element and @github/image-crop-element also use yarnpkg.com so I think it's okay...

I hope someone else has an answer!

Copy link
Contributor

Choose a reason for hiding this comment

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

Someone probably used npm-cli to add it originally but the docs say to use yarn - I don't think it matters but it's probably better using yarn while that's the documented tooling.

@khiga8
Copy link
Contributor

khiga8 commented Mar 8, 2022

You may want to rebase and rerun npm run prepare, which will regenerate the primer_view_components.js files.

jonrohan
jonrohan previously approved these changes Mar 9, 2022
@lindseywild lindseywild force-pushed the update-auto-complete-package branch from f36934c to 90a43c6 Compare March 9, 2022 11:40
@lindseywild lindseywild changed the title chore: bumps autocomplete element package to 3.1.0 feat: bumps autocomplete element package, updates API Mar 9, 2022
@lindseywild lindseywild dismissed jonrohan’s stale review March 10, 2022 14:38

Lots has changed since this was approved, will re-request once it's actually ready!

@lindseywild
Copy link
Contributor Author

@owenniblock Thanks for making the API updates! Just a note so we remember to update the Storybook stories and tests once we're happy with the changes 😄

Copy link
Contributor

@khiga8 khiga8 left a comment

Choose a reason for hiding this comment

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

I added a couple comments as someone very new to the autocomplete work! Thanks for starting this work 💪

I agree with @lindseywild suggestion of explaining why autofocus is bad, maybe in the @accessibility section. In addition, we might want to explain the purpose of the optional clear button.

app/components/primer/beta/auto_complete.rb Outdated Show resolved Hide resolved
app/components/primer/beta/auto_complete.rb Outdated Show resolved Hide resolved
app/components/primer/beta/auto_complete.rb Show resolved Hide resolved
@owenniblock
Copy link
Contributor

We need to make some changes. Currently the Invite a member to actions dialog looks like this:

Dialog containing AutoComplete control, icon sits above the input and right side of input has unexpected border-radius

To fix the icon issue (icon sits above input) - we can move the icon below the input in the markup (it's aria-hidden so I think that should be fine).

The border-radius needs some CSS changes:

.input-group .form-control:first-child, .input-group-button:first-child .btn {
border-top-right-radius: 0;
border-bottom-right-radius: 0;
}

This element is no longer the first-child because we've added a label.

@mperrotti
Copy link
Contributor

mperrotti commented Mar 11, 2022

Screen Shot 2022-03-11 at 11 06 02 AM
Screen Shot 2022-03-11 at 11 06 15 AM

This diverges from how we're handling input icons and actions in primer/react. We're putting icons and actions inside the text input border:

image

There's a new PR for text input trailing actions: primer/react#1947
We already support icons inside of text inputs, which you can see in the TextInput docs: https://primer.style/react/TextInput

@lindseywild
Copy link
Contributor Author

Thanks, @mperrotti ! This was a work in progress before checking with design just to get something working in the examples - we will definitely make sure that this is correct before going live 😄

@khiga8
Copy link
Contributor

khiga8 commented Mar 24, 2022

I removed all the position: relative that were being set in the examples because I don't know what it's for: cf8cfd5.

If it's actually necessary, feel free to add it back in, but maybe it should be a default part of the API.

@lindseywild
Copy link
Contributor Author

@khiga8 @owenniblock @smockle I believe all of the work is now complete in this PR - If you all would be so kind as to check it over, review the docs and that they make sense, any other nitpicks... Please let me know!

@lindseywild lindseywild changed the title WIP - feat: bumps autocomplete element package, updates API feat: bumps autocomplete element package, updates API Mar 25, 2022
@khiga8
Copy link
Contributor

khiga8 commented Mar 25, 2022

I'd encourage everyone to double check swaps now that the API has been updated with new CSS classes.

We'll need to make a couple minor adjustments.

For my sponsorship swap I noticed:

  • Width is overflowing outside constraining box when stacked.
  • When there's a form-group parent, any nested form-control turns grey by default but white on focus. However, autocomplete has an embedded icon area outside the input needs which also change to white. Maybe we need to define something like:
.form-group {
  .autocomplete-body&:focus-within {
    background-color: var(--color-canvas-default);
  }
}

screenshot of swapped autocomplete with odd styles

@lindseywild
Copy link
Contributor Author

I added a to do list at the top of the draft PR in dotcom of what is left and to make sure we verify all instances are accurate!

@smockle
Copy link
Member

smockle commented Mar 25, 2022

From @khiga8 in #1050 (comment):

I'd encourage everyone to double check swaps now that the API has been updated with new CSS classes.

Super advice! I found numerous issues. I pushed https://github.com/github/github/commit/78e62821b19f42fdd3c56a02cd1a1f51269208cb to address a red-screen-of-death caused by a required/missing with_icon update, then I opened https://github.com/github/github/pull/214426 with style & polish updates.

@khiga8 khiga8 marked this pull request as ready for review March 28, 2022 16:33
@khiga8
Copy link
Contributor

khiga8 commented Mar 28, 2022

Hello @primer/rails-reviewers! This is ready for review.

We have a branch in dotcom linked to these changes and have confirmed all instances look good: https://github.com/github/github/pull/214414. Through our swaps, we discovered additional CSS changes need to be made so we'll temporarily house those in dotcom while we work on getting it in Primer CSS.

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.

LGTM!

@lindseywild lindseywild merged commit 38584f1 into main Mar 29, 2022
@lindseywild lindseywild deleted the update-auto-complete-package branch March 29, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants