-
Notifications
You must be signed in to change notification settings - Fork 15
fix: Prevent an error if no sticky search button is used. #2055
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
base: main
Are you sure you want to change the base?
Conversation
subs.ft.com heavily modifies the header. It uses a sticky header without search interface. Generally we don't support this level of customisation for components, and would expect things could break in minor/patch releases without notice. I'd have recommended creating a custom component here from scratch. We might want to consider an Origami header component that centres a logo and allows for custom items left/right, but it's unlikely to make it on our roadmap soon. https://origami.ft.com/getting-started/customisation/ Despite that, this is a small issue, so let us be helpful and check for the presence of search and fail gracefully. We have recommended to pin to the patch release to reduce risk of future changes breaking unsupported customisations.
const searchIcon = headerEl.querySelector( | ||
`[aria-controls="${stickyHeaderId.slice(1)}"]` | ||
); | ||
const isHeaderExpanded = stickyHeaderContainer |
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.
Prettier. This is a one line change, here. isHeaderExpanded
is false if stickyHeaderContainer
is not found.
</div> | ||
</div> | ||
); | ||
return <></>; |
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.
Should all of this been removed?
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.
Oh woops 😅 Yeah
@notlee is this still needed? 😅 |
@j-mes Yeah, in that it's more resilient, but no one is waiting on it. Thanks for the reminder. I'm probably not going to get to this before leaving so feel free to pick up or close |
subs.ft.com heavily modifies the header. It uses a sticky header without search interface.
Generally we don't support this level of customisation for components, and would expect things could break in minor/patch releases without notice. I'd have recommended creating a custom component here from scratch. We might want to consider an Origami header component that centres a logo and allows for custom items left/right, but it's unlikely to make it on our roadmap soon. https://origami.ft.com/getting-started/customisation/
Despite that, this is a small issue, so let us be helpful and check for the presence of search and fail gracefully. We have recommended to pin to the patch release to reduce risk of future changes breaking unsupported customisations.
Closes: #2050