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): "medium-sized" search input height #4237

Merged
merged 7 commits into from
Oct 9, 2019
Merged

feat(search): "medium-sized" search input height #4237

merged 7 commits into from
Oct 9, 2019

Conversation

AlexanderLyon
Copy link
Contributor

Closes #3457

Adds a "medium" size for search input with a height of 40px, as well as updated documentation.

Changelog

Changed

  • _search.scss
  • search.config.js
  • search/README.md

image

@AlexanderLyon AlexanderLyon requested a review from a team as a code owner October 7, 2019 03:57
@ghost ghost requested review from abbeyhrt and jnm2377 October 7, 2019 03:57
@AlexanderLyon
Copy link
Contributor Author

@laurenmrice

@AlexanderLyon AlexanderLyon changed the title "Medium-sized" search input feat(search): "medium-sized" search input height Oct 7, 2019
@netlify
Copy link

netlify bot commented Oct 7, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit 18bc529

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

@netlify
Copy link

netlify bot commented Oct 7, 2019 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 18bc529

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

asudoh
asudoh reviewed Oct 7, 2019
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for jumping in @AlexanderLyon! Some comments:

  • Can you make the suffix lg because we used have lg whose size was 40px? (Side note: The default (48px) variant is called xl)
  • Can you make sure Close16 is used?

@netlify
Copy link

netlify bot commented Oct 7, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 18bc529

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

@asudoh asudoh mentioned this pull request Oct 7, 2019
Merged
laurenmrice
laurenmrice requested changes Oct 7, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good in Vanilla, we do need to have the close icon popup if you start typing in the field, currently it is not showing. You can reference the xlg and sm versions.

For react I don't see a way to change the search to the lg version. I see the search is xlg by default and there is a knob option for small, but there should also be a knob option for lg.

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 7, 2019

@laurenmrice I think this is a vanilla only contribution

@laurenmrice
Copy link
Member

Oh okay thanks Josefina! @AlexanderLyon Do you feel comfortable making this in React as well? I just noticed the file wasn't updated for React, if not that is okay and I can open a separate issue for someone to tackle.

@AlexanderLyon
Copy link
Contributor Author

@laurenmrice I can definitely give it a shot later today!

jnm2377
jnm2377 suggested changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! It's looking really good. Just a few comments:

  • Looks like the clear search button is there, but the svg blends in with the background, so we just need to update the svg color.

Screen Shot 2019-10-07 at 10 55 06 AM

  • Like @asudoh said, let's update the suffix to be lg and the scss selector to be .#{$prefix}--search--lg. Sorry for the confusion. I know the design specs say medium but this would technically be our lg version since our normal search bar is xl. I think it's ok if the label says "Medium search" (search.config.js: line 32)

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 7, 2019

@laurenmrice @AlexanderLyon actually, just realized it looks like Akira has a draft pr in progress for react changes that relies on this fix #4238

So I think you can just fix the style changes we asked for and it should be good to go. We'll make sure your PR gets merged in before Akira's since his PR relies on this. 👍

@AlexanderLyon
Copy link
Contributor Author

@jnm2377 sounds good, I should have this ready to go by tomorrow 👍

AlexanderLyon and others added 4 commits October 7, 2019 20:32
@AlexanderLyon
feat(search): add styles for 40px search input
f413089
@dakahn @AlexanderLyon
chore(react): add component name to deprecation warning for toolbar (c…
2fc1446 
…arbon-design-system#4229)

* chore(Toolbar/ToobarSearch): add component name to deprecation warning

* docs(Toolbar): add deprecation decorator to Toolbar story

* docs(Toolbar): un-add deprecation decorator
@AlexanderLyon
feat(search): add medium configuration
f5ddbeb
@AlexanderLyon
feat(search): add medium description to README
e32a17d
asudoh
asudoh approved these changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @AlexanderLyon for the updates!

@AlexanderLyon
Copy link
Contributor Author

@jnm2377 @asudoh - requested changes have been made! Let me know if you'd rather change the label from "medium" to "large", as this may also require changing the descriptions for the existing sizes too

@asudoh asudoh requested a review from jnm2377 October 8, 2019 00:53
jnm2377
jnm2377 approved these changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@jnm2377 jnm2377 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! thanks! 🎉

laurenmrice
laurenmrice approved these changes Oct 8, 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 🎉

abbeyhrt
abbeyhrt approved these changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

@laurenmrice laurenmrice mentioned this pull request Oct 8, 2019
Closed
@joshblack joshblack merged commit fb66d5b into carbon-design-system:master Oct 9, 2019
@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

@asudoh asudoh asudoh approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

@laurenmrice laurenmrice laurenmrice approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Search] add 40px search size
7 participants
@AlexanderLyon @jnm2377 @laurenmrice @asudoh @abbeyhrt @joshblack @dakahn