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.

Sign up for GitHub

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

Jump to bottom

feat(Search): introduce large search back #4238

Merged
merged 6 commits into from
Oct 11, 2019
Merged

feat(Search): introduce large search back #4238

merged 6 commits into from
Oct 11, 2019

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Oct 7, 2019

Refs #3457.

The styling code is covered by: #4237

Changelog

New

  • lg search variant (back).
  • size prop which takes xl, lg, or sm.

Changed

  • Deprecated small prop for the sake of size.

Testing / Reviewing

Testing should make sure <Search> and <TableToolbarSearch> are not broken.

@netlify
Copy link

netlify bot commented Oct 7, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit aa9c3c2

https://deploy-preview-4238--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Oct 7, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit aa9c3c2

https://deploy-preview-4238--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Oct 7, 2019 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit aa9c3c2

https://deploy-preview-4238--carbon-elements.netlify.com

@jnm2377 jnm2377 mentioned this pull request Oct 7, 2019
Merged
@asudoh asudoh requested a review from emyarod October 8, 2019 21:41
@asudoh asudoh marked this pull request as ready for review October 8, 2019 21:42
@asudoh asudoh requested a review from a team as a code owner October 8, 2019 21:42
emyarod
emyarod approved these changes Oct 9, 2019
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me now that #4237 is merged in, pending design approval

laurenmrice
laurenmrice requested changes Oct 9, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

The close icon is 2px to the left when the hover/focus is activated around it. It should be in the complete center when that happens.

Screen Shot 2019-10-09 at 11 44 38 AM

@asudoh
Copy link
Contributor Author

asudoh commented Oct 10, 2019

@laurenmrice Good catch - Fixed.

laurenmrice
laurenmrice reviewed Oct 10, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

In Vanilla it still needs to come over one to the left, its slightly off. Then we should be good. I couldn't review React yet because the build failed.

@asudoh
Copy link
Contributor Author

asudoh commented Oct 10, 2019

In Vanilla it still needs to come over one to the left, its slightly off.

@laurenmrice Is it about the position of X icon, or something else...? Thanks!

@laurenmrice
Copy link
Member

@asudoh Yes sorry, the x icon needs to come over a pixel to the left, its still not in the center.

@asudoh
Copy link
Contributor Author

asudoh commented Oct 10, 2019

No worries @laurenmrice, thank you for clarifying - Fixed.

laurenmrice
laurenmrice approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks good ! thank you 🙌

@asudoh asudoh merged commit 0649645 into carbon-design-system:master Oct 11, 2019
@asudoh asudoh deleted the search-lg branch October 11, 2019 22:45
@laurenmrice laurenmrice mentioned this pull request Nov 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

@laurenmrice laurenmrice laurenmrice approved these changes

Assignees
No one assigned
Labels
status: ready for review 👀 status: visual review 🔍
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@asudoh @laurenmrice @emyarod @jnm2377