-
Notifications
You must be signed in to change notification settings - Fork 0
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
LIIKUNTA-584 | feat: update to HDS v3.1.0, use Header in Navigation component #131
Conversation
2fe7ca5
to
b09cefa
Compare
b09cefa
to
294a6c1
Compare
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.
I think the changes look OK. I added a minor note and asked @melniiv to give his comments too.
// WORKAROUND: Trying to set var(--header-max-width) to var(--breakpoint-xl) instead of | ||
// the default 1440px. Not an ideal solution, this may break e.g. if HDS | ||
// Header's subcomponents change their class names. | ||
.maxWidthXl { | ||
div[class^='HeaderActionBar-module_headerActionBar_'], | ||
div[class*=' HeaderActionBar-module_headerActionBar_'], | ||
nav[class^='HeaderNavigationMenu-module_headerNavigationMenu_'], | ||
nav[class*=' HeaderNavigationMenu-module_headerNavigationMenu_'] { | ||
margin: 0 auto; | ||
max-width: var(--breakpoint-xl); | ||
} | ||
} |
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.
@melniiv what do you think about this?
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.
i think that when HDS makes it configurable we refactor that. Discussed with Kari
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.
Yes, this was discussed with HDS team on Slack #design-system thread on Friday Nov 10th, 2023. It seemed HDS team was at least initially open to the idea of making the maximum header width configurable, but let's see!
src/core/navigation/Navigation.tsx
Outdated
const findLanguage = (languageCode: string) => | ||
languages.find( | ||
(language) => language.code?.toLowerCase() === languageCode.toLowerCase(), | ||
); |
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.
Would be better moved away from the component to make the actual component cleaner. It's usually a bad practise to have functions inside functions: they are not reusable then and they become statefull instead of stateless. The component declares the function everytime the component is rerendered.
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.
Moved findLanguage to own function in force push
294a6c1
to
5ee8afd
Compare
From HDS v3.0.0 release notes: > **Breaking** > - [Navigation] Removed, use new Header instead > - [Footer] Redesigned and rebuilt > - [StatusLabel, Tag] Rectangular versions removed > - [LinkBox] Property name withBorder changed to border > - [Koros] Renamed variants Wave to Vibration and Storm to Wave > ... > **Changed** > - [Koros] Rename koros flipHorizontal to flipVertical steps: - set HCRC version to 1.0.0-alpha229 - update hds-(core|design-tokens|react) from ^2.17.0 to ^3.1.0 - icons - export IconEuroSign - export IconAtSign - export IconCalendarEvent - export IconWhatsapp - export HDS v2 icon names as aliases for v3 icons for backward compatibility - sort icon exports alphabetically - replace RoundedTag with Tag because "[StatusLabel, Tag] Rectangular versions removed" - rename flipHorizontal to fipVertical because "[Koros] Rename koros flipHorizontal to flipVertical" - rename hero.module.scss's &.wave to &.vibration because "[Koros] Renamed variants Wave to Vibration and Storm to Wave" - rename hero.module.scss's &.storm to &.wave because "[Koros] Renamed variants Wave to Vibration and Storm to Wave" - switch react-helsinki-headless-cms's Navigation component to use HDS v3's Header because "[Navigation] Removed, use new Header instead" - deprecate variant property from NavigationProps as unused sources: - HDS v3 migration guide: - https://hds.hel.fi/getting-started/tutorials/migrate-to-3.0.0/ - HDS releases v3.0.0 and v3.1.0: - https://github.com/City-of-Helsinki/helsinki-design-system/releases - /tag/v3.0.0 - /tag/v3.1.0 refs LIIKUNTA-584
5ee8afd
to
ee3edb2
Compare
Rebased on main and force pushed |
- upgrade to hds-react 3.1.0 - upgrade to react-helsinki-headless-cms 1.0.0-alpha229 - UI components: - <Footer> was rebuilt in HDS v3 so needed to make changes to it - rename flipHorizontal to flipVertical per HDS v3 release notes - tests: - update snapshots - NOTE: **snapshot updating** updated **suspiciously** e.g. "tag_hds-tag__label__I6mc8" to **"undefined"**, maybe there's something broken? - fix browser tests because language selector changed in HCRC's Navigation component which is now actually using HDS v3's Header component underneath - storybook: - fix "yarn build-storybook" and "yarn storybook" failing under packages/components - NOTE: the storybook looks broken but at least it builds and runs - add "crypto-browserify" because it was needed by HDS v3 - fix graphql code generation creating unusable enumeration values by making sure the enum keys don't start with a digit - miscellaneous fixes - fix use(Events|Hobbies|Sports)RHHCConfig.tsx by adding missing second argument organisationPrefixes with value CITY_OF_HELSINKI_LINKED_EVENTS_ORGANIZATION_PREFIXES to getEventCardProps See react-helsinki-headless-cms 1.0.0-alpha229 PR: City-of-Helsinki/react-helsinki-headless-cms#131 refs LIIKUNTA-584
- upgrade to hds-react 3.1.0 - upgrade to react-helsinki-headless-cms 1.0.0-alpha229 - UI components: - <Footer> was rebuilt in HDS v3 so needed to make changes to it - rename flipHorizontal to flipVertical per HDS v3 release notes - tests: - update snapshots - NOTE: **snapshot updating** updated **suspiciously** e.g. "tag_hds-tag__label__I6mc8" to **"undefined"**, maybe there's something broken? - fix browser tests because language selector changed in HCRC's Navigation component which is now actually using HDS v3's Header component underneath - storybook: - fix "yarn build-storybook" and "yarn storybook" failing under packages/components - NOTE: the storybook looks broken but at least it builds and runs - add "crypto-browserify" because it was needed by HDS v3 - fix graphql code generation creating unusable enumeration values by making sure the enum keys don't start with a digit See react-helsinki-headless-cms 1.0.0-alpha229 PR: City-of-Helsinki/react-helsinki-headless-cms#131 refs LIIKUNTA-584
- upgrade to hds-react 3.1.0 - upgrade to react-helsinki-headless-cms 1.0.0-alpha229 - UI components: - <Footer> was rebuilt in HDS v3 so needed to make changes to it - rename flipHorizontal to flipVertical per HDS v3 release notes - tests: - update snapshots - NOTE: **snapshot updating** updated **suspiciously** e.g. "tag_hds-tag__label__I6mc8" to **"undefined"**, maybe there's something broken? - fix browser tests because language selector changed in HCRC's Navigation component which is now actually using HDS v3's Header component underneath - storybook: - fix "yarn build-storybook" and "yarn storybook" failing under packages/components - NOTE: the storybook looks broken but at least it builds and runs - add "crypto-browserify" because it was needed by HDS v3 - fix graphql code generation creating unusable enumeration values by making sure the enum keys don't start with a digit See react-helsinki-headless-cms 1.0.0-alpha229 PR: City-of-Helsinki/react-helsinki-headless-cms#131 refs LIIKUNTA-584
Description
Testable at (PR #534):
feat: update to HDS v3.1.0, use Header in Navigation component
From HDS v3.0.0 release notes:
steps:
compatibility
"[StatusLabel, Tag] Rectangular versions removed"
"[Koros] Rename koros flipHorizontal to flipVertical"
"[Koros] Renamed variants Wave to Vibration and Storm to Wave"
"[Koros] Renamed variants Wave to Vibration and Storm to Wave"
HDS v3's Header because
"[Navigation] Removed, use new Header instead"
sources:
refs LIIKUNTA-584
Issues
Closes
LIIKUNTA-584
Related
Testing
Automated tests
Manual testing
Screenshots
Additional notes