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.

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

feat: o-header subnav primary link styles #1853

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

Roberta-M
Copy link
Contributor

@Roberta-M Roberta-M commented Oct 18, 2024

Describe your changes

The App has a requirement to have a sub navigation where link items have 'primary' styles and are capitalised with font weight 600. Currently, the .o-header__nav-link--primary class provides these styles for primary navigation links, but no equivalent modifier exists for the sub navigation .o-header__subnav-link.

To address this, this introduces .o-header__subnav-link--primary style modifier for the sub navigation. Additionally, the shared styles are now extracted into a reusable mixin.
Now updated to apply the modifier class directly to .o-header__subnav--primary instead of individually to the child item links. This also includes background colour update to use paper colour so it is styled more like the main navigation.

Issue ticket number and link

Jira ticket: https://financialtimes.atlassian.net/jira/software/c/projects/AT/boards/1853?selectedIssue=AT-6340

Link to Figma designs

Figma: https://www.figma.com/design/qYx7j1N3c2qIjJpknCTyPu/De-Fruiting-the-App-%F0%9F%8D%93?node-id=661-91803&node-type=section&t=xyFXvqn98yEKoayT-0

A few small questions:

  • Is primary in .o-header__subnav-link--primary a suitable name for this?
  • Is it worth updating Storybook to document the new style modifier?

Screenshot

.o-header__subnav-link--primar style in use within the App Life & Arts navigation:

Screenshot 2024-10-18 at 17 33 45

Checklist before requesting a review

  • I have applied percy label for o-[COMPONENT] or chromatic label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md
  • If it is a new feature, I have added thorough tests.
  • I have updated relevant docs.
  • I have updated relevant env variables in Doppler.

@Roberta-M Roberta-M requested a review from a team as a code owner October 18, 2024 16:43
@notlee notlee temporarily deployed to origami-webs-feature-su-efmglh October 18, 2024 16:47 Inactive
@Roberta-M Roberta-M force-pushed the feature/sub-navigation-link-primary-style branch from 3974967 to 35f1ef1 Compare October 23, 2024 11:55
@notlee notlee temporarily deployed to origami-webs-feature-su-rqefxm October 23, 2024 11:59 Inactive
@Roberta-M Roberta-M force-pushed the feature/sub-navigation-link-primary-style branch from 35f1ef1 to b34235a Compare October 23, 2024 12:01
@notlee notlee temporarily deployed to origami-webs-feature-su-rqefxm October 23, 2024 12:02 Inactive
The App has a requirement to have a sub navigation where link items
have 'primary' styles and are capitalised with font weight 600. The
navigation also requires the background image to use `paper` colour
instead of the current default `white-60`. Esentially we want the sub
navigation to look like a primary navigation but still have the
scroll behaviour of the sub navigation.

This introduces `o-header__subnav--primary` modifier which will provide
the required style modifications for background colour and text style.

Additionally since the text styles are now being using in two places, I've
extracted these out to a `_oHeaderPrimaryLink` mixin.
@Roberta-M Roberta-M force-pushed the feature/sub-navigation-link-primary-style branch from b34235a to d9ca368 Compare October 23, 2024 12:26
@notlee notlee temporarily deployed to origami-webs-feature-su-rqefxm October 23, 2024 12:27 Inactive
@Roberta-M
Copy link
Contributor Author

Roberta-M commented Oct 23, 2024

@notlee That's a great idea to use a modifier class higher up on the o-header-subnav instead, and also include the background colour change. I've now made the suggested changes, so it's ready for another review 🙏

This did also make me question if the App could be using the main navigation markup instead of the sub navigation from o-header. The main issue with that approach is that we don't have the js scroll behaviour on the main navigation, that would be a bigger change to make, so this modifier on sub nav approach feels a bit simpler.

@Roberta-M Roberta-M requested a review from notlee October 23, 2024 12:38
@notlee notlee merged commit 421eb30 into main Oct 24, 2024
8 checks passed
@notlee notlee deleted the feature/sub-navigation-link-primary-style branch October 24, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants