feat(Header): Convert Header to CSS modules behind team feature flag #5192

hussam-i-am · 2024-10-29T21:12:03Z

Closes https://github.com/github/primer/issues/4138

Changelog

New

Changed

Convert Header to CSS modules behind team feature flag

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-10-29T21:12:07Z

🦋 Changeset detected

Latest commit: f2f1fcc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-10-29T21:12:38Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

github-actions · 2024-10-29T21:15:10Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	97.5 KB (+0.11% 🔺)
packages/react/dist/browser.umd.js	97.83 KB (+0.07% 🔺)

primer-integration · 2024-10-29T21:49:57Z

🟢 golden-jobs completed with status success.

…rimer/react into hussam-i-am/css_modules_header

…am/css_modules_header

hussam-i-am · 2024-10-30T16:54:06Z

Updated the unit and storybook tests to verify className, as well as added support for as prop for Header since it was failing in the integration build

langermank · 2024-10-30T17:22:30Z

packages/react/src/Header/Header.tsx

+      className={clsx(className, enabled && classes.HeaderItem, enabled && rest.full && classes.Full)}
+      {...rest}


For variant type props, we're using data-attributes for styling instead of classnames. You can read a little bit about that in the CSS authoring guide.

So for the full prop, instead of a classname it would look something like this:

Suggested change

className={clsx(className, enabled && classes.HeaderItem, enabled && rest.full && classes.Full)}

{...rest}

className={clsx(className, enabled && classes.HeaderItem}

data-full={full}

{...rest}

And then in CSS

&:where([data-full]) { .... } If you check out the ButtonBase component you can see this in action 😄

Nice, makes a lot of sense, updated!

primer-integration · 2024-10-30T18:03:07Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/349206

joshblack

Sorry I was late on this! Just had a couple comments for this that I think would be good to cover

joshblack · 2024-10-31T15:44:10Z

packages/react/src/Header/Header.tsx

+    const isReactRouter = typeof to === 'string'
+    if (isReactRouter) {
+      // according to their docs, NavLink supports aria-current:
+      // https://reacttraining.com/react-router/web/api/NavLink/aria-current-string
+      return {'aria-current': 'page'}
+    } else {
+      return {}
+    }


I think this logic will need to be ported over into the HeaderLink so that it works with the flag enabled/disabled. Right now, it will only be applied when the styled component is used if I understand right 👀

joshblack · 2024-10-31T15:45:26Z

packages/react/src/Header/Header.tsx

+      data-full={rest.full}
+      {...rest}


I think a challenge with this is that full will be an attribute when the flag is enabled and will give us an unexpected attribute error, e.g.

I think we should either update the styled component to use the data attribute pattern or only pass along full when the flag is enabled, if that makes sense.

hussam-i-am added 2 commits October 29, 2024 21:03

feat(Header): Convert Header to CSS modules behind team feature flag

396f652

dev stories

98b3d88

hussam-i-am self-assigned this Oct 29, 2024

hussam-i-am requested a review from a team as a code owner October 29, 2024 21:12

hussam-i-am requested a review from langermank October 29, 2024 21:12

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 29, 2024

hussam-i-am temporarily deployed to github-pages October 29, 2024 21:14 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5192 October 29, 2024 21:14 Inactive

prettier fix

299c161

hussam-i-am temporarily deployed to github-pages October 29, 2024 21:23 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5192 October 29, 2024 21:24 Inactive

update types

61a7fcc

hussam-i-am temporarily deployed to github-pages October 29, 2024 22:02 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5192 October 29, 2024 22:02 Inactive

Merge branch 'main' into hussam-i-am/css_modules_header

368f264

hussam-i-am temporarily deployed to github-pages October 30, 2024 15:25 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5192 October 30, 2024 15:26 Inactive

hussam-i-am added 3 commits October 30, 2024 16:51

Fix missing 'as' prop and update tests

74e6bf3

Merge branch 'hussam-i-am/css_modules_header' of https://github.com/p…

9765046

…rimer/react into hussam-i-am/css_modules_header

Merge branch 'main' of https://github.com/primer/react into hussam-i-…

2796ef0

…am/css_modules_header

hussam-i-am temporarily deployed to github-pages October 30, 2024 16:54 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5192 October 30, 2024 16:54 Inactive

hussam-i-am added 2 commits October 30, 2024 17:07

format

aba6715

Merge branch 'main' into hussam-i-am/css_modules_header

dc2773d

hussam-i-am temporarily deployed to github-pages October 30, 2024 17:11 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5192 October 30, 2024 17:11 Inactive

langermank reviewed Oct 30, 2024

View reviewed changes

replace full class with data attr

f2f1fcc

hussam-i-am deployed to github-pages October 30, 2024 17:41 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5192 October 30, 2024 17:41 View deployment

hussam-i-am requested review from jonrohan and joshblack October 30, 2024 20:31

jonrohan approved these changes Oct 30, 2024

View reviewed changes

hussam-i-am added this pull request to the merge queue Oct 30, 2024

Merged via the queue into main with commit cbeed21 Oct 30, 2024
43 checks passed

hussam-i-am deleted the hussam-i-am/css_modules_header branch October 30, 2024 22:24

primer bot mentioned this pull request Oct 30, 2024

Release tracking (37.1.0) #5166

Merged

joshblack reviewed Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Header): Convert Header to CSS modules behind team feature flag #5192

feat(Header): Convert Header to CSS modules behind team feature flag #5192

hussam-i-am commented Oct 29, 2024 •

edited

Loading

changeset-bot bot commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 •

edited

Loading

primer-integration bot commented Oct 29, 2024 •

edited

Loading

hussam-i-am commented Oct 30, 2024

langermank Oct 30, 2024

hussam-i-am Oct 30, 2024

primer-integration bot commented Oct 30, 2024

joshblack left a comment

joshblack Oct 31, 2024

joshblack Oct 31, 2024

		className={clsx(className, enabled && classes.HeaderItem, enabled && rest.full && classes.Full)}
		{...rest}

feat(Header): Convert Header to CSS modules behind team feature flag #5192

feat(Header): Convert Header to CSS modules behind team feature flag #5192

Conversation

hussam-i-am commented Oct 29, 2024 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Oct 29, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 29, 2024

github-actions bot commented Oct 29, 2024 • edited Loading

size-limit report 📦

primer-integration bot commented Oct 29, 2024 • edited Loading

hussam-i-am commented Oct 30, 2024

langermank Oct 30, 2024

Choose a reason for hiding this comment

hussam-i-am Oct 30, 2024

Choose a reason for hiding this comment

primer-integration bot commented Oct 30, 2024

joshblack left a comment

Choose a reason for hiding this comment

joshblack Oct 31, 2024

Choose a reason for hiding this comment

joshblack Oct 31, 2024

Choose a reason for hiding this comment

hussam-i-am commented Oct 29, 2024 •

edited

Loading

changeset-bot bot commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

primer-integration bot commented Oct 29, 2024 •

edited

Loading