[Navigation] Consolidate se23 logic and styles #10213

laurkim · 2023-08-24T16:48:15Z

WHY are these changes introduced?

Resolves #9952.

WHAT is this pull request doing?

Consolidates se23 beta styles and logic for Navigation.

How to 🎩

Storybook
Prod Storybook

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

laurkim · 2023-08-24T16:50:31Z

polaris-react/src/components/Navigation/Navigation.scss

@@ -379,7 +315,7 @@ $nav-max-width: 360px;
    // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
    @include no-focus-ring;
    color: var(--p-color-text-primary);
-    background-color: var(--p-color-bg-app-active);
+    background-color: var(--p-color-bg-app-selected);


Changed due to specificity issues.

laurkim · 2023-08-24T16:50:49Z

polaris-react/src/components/Navigation/Navigation.scss

  }

  // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
  @include focus-ring;

  &:hover,
  &:focus-visible {
-    background-color: var(--p-color-bg-app-hover);


Removed due to specificity issues.

laurkim · 2023-08-24T16:54:32Z

polaris-react/src/components/Navigation/Navigation.scss

-  @media #{$p-breakpoints-md-up} {
-    margin-right: var(--p-space-3);
-  }
+  margin-right: var(--p-space-1);


Removed breakpoint margin styling due to specificity issues.

laurkim · 2023-08-24T16:55:14Z

polaris-react/src/components/Navigation/Navigation.scss

-  .SecondaryActions {
-    margin-right: 0;
-  }


Removed this margin due to specificity issues.

laurkim · 2023-08-24T16:55:59Z

polaris-react/src/components/Navigation/Navigation.scss

@@ -690,7 +544,6 @@ $nav-max-width: 360px;
  &:active {
    // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
    @include no-focus-ring;
-    background: var(--p-color-bg-app-active);


Removed due to specificity issues.

laurkim · 2023-08-24T16:56:27Z

polaris-react/src/components/Navigation/Navigation.scss

      }
    }

    &:focus-visible {
      // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
      @include focus-ring($style: 'focused');
-      color: var(--p-color-text);


Removed due to specificity issues.

laurkim · 2023-08-24T16:56:36Z

polaris-react/src/components/Navigation/Navigation.scss

@@ -791,51 +629,40 @@ $nav-max-width: 360px;
    @media #{$p-breakpoints-md-up} {
      font-size: var(--p-font-size-100);
      line-height: 1;
-      padding-left: var(--p-space-10);


Removed due to specificity issues.

laurkim · 2023-08-24T16:57:41Z

polaris-react/src/components/Navigation/Navigation.scss

@@ -1128,6 +922,10 @@ $nav-max-width: 360px;
    // stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
    @include focus-ring($style: 'focused');
  }
+
+  &:active {
+    background-color: var(--p-color-bg-app-active);


Added due to specificity issues.

laurkim · 2023-08-24T16:58:16Z

polaris-react/src/components/Navigation/_variables.scss

  &:active,
  &:active:hover {
    color: var(--p-color-text);
-    background-color: var(--p-color-bg-app-active);
+    background-color: var(--p-color-bg-app-selected);


Changed due to specificity issues.

laurkim · 2023-08-24T16:58:24Z

polaris-react/src/components/Navigation/_variables.scss

  @media #{$p-breakpoints-md-up} {
    font-size: var(--p-font-size-200);
    font-weight: var(--p-font-weight-medium);
    line-height: var(--p-font-line-height-5);
-    padding-left: var(--p-space-3);


Removed due to specificity issues.

sophschneider

🤩

sophschneider · 2023-08-24T17:52:10Z

btw I tophatted xs screen, keyboard navigation, and responsive screen and everything had parity with production! I did notice some bugs in production on xs screens, I've logged them here #10218 since it isn't related to this consolidation

* next: (87 commits) [ButtonGroup] Removed `segmented` boolean prop and replaced with `variant` (#9997) [LegacyTabs] Consolidate se23 styles and logic (#10231) [v12] Update changesets (#10232) [LegacyFilters] Consolidate se23 styles and logic (#10217) [Banner] Change status prop to tone (#10206) Replace icon highlight color in box stories (#10226) [TopBar] Consolidate se23 styles and logic (#10221) [Filters] consolidate se23 logic and styles (#10178) [Icon] Update props (#10220) [ButtonGroup] Update variant plain prop name (#10222) [Button] Remove connectedDisclosure (#10182) [LegacyCard] Consolidate se23 styles and logic (#10207) [Navigation] Consolidate se23 logic and styles (#10213) [Frame][ContextualSaveBar] Consolidate se23 styles and logic (#10196) [OptionList] consolidate se23 styles and logic (#10177) [Page] Consolidate se23 styles and logic (#10187) [DataTable] Consolidate conditional logic (#10169) [Thumbnail] Consolidate conditional logic (#10171) [ResourceItem] Consolidate conditional logic (#10172) [FullscreenBar] Consolidate conditional logic (#10173) ...

### WHY are these changes introduced? Resolves #9952. ### WHAT is this pull request doing? Consolidates se23 beta styles and logic for `Navigation`. ### How to 🎩 [Storybook](https://5d559397bae39100201eedc1-lecmsuzwza.chromatic.com/?path=/story/all-components-navigation--default) [Prod Storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-navigation--default&globals=polarisSummerEditions2023:true) 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide

### WHY are these changes introduced? Resolves Shopify#9952. ### WHAT is this pull request doing? Consolidates se23 beta styles and logic for `Navigation`. ### How to 🎩 [Storybook](https://5d559397bae39100201eedc1-lecmsuzwza.chromatic.com/?path=/story/all-components-navigation--default) [Prod Storybook](https://storybook.polaris.shopify.com/?path=/story/all-components-navigation--default&globals=polarisSummerEditions2023:true) 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### 🎩 checklist - [x] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide

laurkim self-assigned this Aug 24, 2023

laurkim added the #gsd:36579 label Aug 24, 2023

laurkim commented Aug 24, 2023

View reviewed changes

laurkim force-pushed the lo/rm-uplift-navigation branch from 1dbb431 to eb701c9 Compare August 24, 2023 16:54

laurkim commented Aug 24, 2023

View reviewed changes

[Navigation] Consolidate se23 logic and styles

b064566

laurkim force-pushed the lo/rm-uplift-navigation branch from eb701c9 to b064566 Compare August 24, 2023 17:00

laurkim marked this pull request as ready for review August 24, 2023 17:22

laurkim requested review from gwyneplaine and sophschneider August 24, 2023 17:22

sophschneider approved these changes Aug 24, 2023

View reviewed changes

laurkim merged commit 9122832 into next Aug 24, 2023
16 checks passed

laurkim deleted the lo/rm-uplift-navigation branch August 24, 2023 17:56

sam-b-rose mentioned this pull request Sep 19, 2023

Remove nav level rule #10568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Navigation] Consolidate se23 logic and styles #10213

[Navigation] Consolidate se23 logic and styles #10213

laurkim commented Aug 24, 2023 •

edited

Loading

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

laurkim Aug 24, 2023

sophschneider left a comment

sophschneider commented Aug 24, 2023 •

edited

Loading

[Navigation] Consolidate se23 logic and styles #10213

[Navigation] Consolidate se23 logic and styles #10213

Conversation

laurkim commented Aug 24, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophschneider left a comment

Choose a reason for hiding this comment

sophschneider commented Aug 24, 2023 • edited Loading

laurkim commented Aug 24, 2023 •

edited

Loading

sophschneider commented Aug 24, 2023 •

edited

Loading