Skip to content

Conversation

@gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Sep 10, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5386

Description (What does it do?)

This PR makes changes to our Input element to match some updates from Figma. We now have 4 sizes of Input:

  • small
  • medium
  • large
  • hero

The general style of the Input control was updated to match the changes to the search input, and SearchInput was moved into ol-components. The SearchInput component implements Input at the hero size on the home page and the large size on the search page and channel pages.

Screenshots (if appropriate):

Screenshot 2024-09-11 at 4 28 24 PM
Screenshot 2024-09-11 at 4 29 08 PM
Screen Shot 2024-09-11 at 16 28 44
Screen Shot 2024-09-11 at 16 29 24

How can this be tested?

@gumaerc gumaerc force-pushed the cg/search-box-updates-2 branch from de82b47 to 4be9d0a Compare September 11, 2024 19:47
@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Sep 11, 2024
@ChristopherChudzicki ChristopherChudzicki self-assigned this Sep 12, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

This looks good. I did make a suggestion that I think we should do to keep the size styles more maintainable going forward. Let me know what you think.

Comment on lines 118 to 120
[theme.breakpoints.down("sm")]: {
height: "40px",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the button padding certainly simplifies stuff 👍 Width is better.

I do worry that we will have a hard time keeping the mobile styles in sync. E.g., if someone updates the medium style, will they also remember to update large + breakpoint small?

I do think it would be better to put all the size-related styles into a sizeStyles, and re-use that at the smaller breakpoint (like what we do for buttons).

I think this does it: cg/search-box-updates-2...cg/search-box-updates-2-cc-suggestion#diff-11bf431b91ecb2c9d080ba7edb0f8f9518c9bed7b5fe9cf6154ecb1a835d5ac5L125

Note: In that diff, I made responsive optional and defaults to undefined ~ false. We could default it to "true" (which this PR effectively does) but I think we should consult Steve about that.

@gumaerc gumaerc force-pushed the cg/search-box-updates-2 branch from 43363e6 to a446259 Compare September 13, 2024 18:20
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍 👍

@gumaerc gumaerc merged commit 8afadb3 into main Sep 13, 2024
@odlbot odlbot mentioned this pull request Sep 17, 2024
7 tasks
@odlbot odlbot mentioned this pull request Sep 18, 2024
13 tasks
@odlbot odlbot mentioned this pull request Sep 18, 2024
14 tasks
@odlbot odlbot mentioned this pull request Sep 18, 2024
15 tasks
@rhysyngsun rhysyngsun deleted the cg/search-box-updates-2 branch February 7, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants