-
Notifications
You must be signed in to change notification settings - Fork 14
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
The new o-header
#1777
The new o-header
#1777
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
I haven't had time to do a proper review but I can see this isn't quite ready for release yet. For example, markup changes are classed as a breaking change so we need to ensure this is a major release and has a migration guide.
As I'm off tomorrow I've written up a Jira Ticket to capture some things the Origami team should pay some closer attention to on review
Search bar can be displayed three times. Drawer, sticky and main nav. In production label and controls currently are not properly associated. See this commit for the reference. If you inspect the main input field and it's label you will spot they reference different IDs. I can write up a migration guide, highlighting changes described in this PR. The main change related to the |
@frshwtr you should be able to start up the storybook and go to http://localhost:6006/?path=/story/components-o-header--header-primary&globals=backgrounds:!undefined |
@frshwtr I am not sure if you are QAing the changes I've pushed. Th screenshot you've shared doesn't look right. I can see that the button on your screenshot looks different to what I have locally Please, see the dimension of the search bar and the button was (42px): I have pushed a commit to reduce the icon height, the bar height should be |
@@ -1,5 +1,23 @@ | |||
# Migration Guide | |||
|
|||
## Migrating from v13 to v14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guide is useful in providing updates on the changes to TSX users. o-header also supports HTML and CSS users. Please could you add some details on how I would upgrade my code using this approach?
Migrating from v11 to v12
in this file has some examples of the things to mention. Mostly an example of the overall HTML to use (from the storybook link) and details on what individual sections can be removed/added/updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this. I've addressed in a357342
This looks good now, thanks. |
…e referencing incorect IDs
Describe your changes
Please, follow my commit history while reviewing the PR.
FT header is undergoing designs changes as a part of the initiative to revamp search on ft.com.
Main changes:
type="search"
which is the semantically correct type. We are getting some built-in advantages of usingtype="search"
. It should have a positive effect ona11ty
. More about itx
toclose
Demo
Desktop
Tablet
Phone (375 width). This break-point is not meant to be used because you cannot toggle the search menu, but it's okay to still make it look acceptable
Phone (320 width). This break-point is not meant to be used because you cannot toggle the search menu, but it's okay to still make it look acceptable
Issue ticket number and link
https://financialtimes.atlassian.net/jira/software/c/projects/CON/boards/1061?selectedIssue=CON-3449
Link to Figma designs
https://www.figma.com/design/L6qyUFPPLRmf59OxTkz2VJ/Semantic-Search?node-id=10-2861&m=dev
Checklist before requesting a review
percy
label for o-[COMPONENT] orchromatic
label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md