Update breadcrumbs to new styling #131

BSFishy · 2022-11-21T17:13:59Z

Description

Migrate breadcrumb style to OUI

Issues Resolved

#22

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Matt Provost <provomat@amazon.com>

AMoo-Miki · 2022-12-03T02:25:39Z

This is for v2.0.0, right?

BSFishy · 2022-12-03T20:50:35Z

I think this feature is targeted for 2.0.0, if I'm not mistaken. Although I think this could technically be in a minor release, I think we're focused on pushing directly through to 2.0 right now

KrooshalUX · 2022-12-03T21:49:01Z

This change is already live on dashboards. The work here is to integrate it into OUI. Not sure if that impacts versioning or not. Message ID: ***@***.***>

joshuarrrr · 2022-12-05T21:25:34Z

This is for v2.0.0, right?

From the Dashboards perspective, we'd prefer that this be backported to 1.x, so that we can pull it in to fix some breadcrumb rendering bugs we've discovered in 2.3.0 of OpenSearch Dashboards.

joshuarrrr · 2022-12-05T21:28:01Z

This change is already live on dashboards. The work here is to integrate it into OUI. Not sure if that impacts versioning or not.

That's not quite true - this is a re-implementation/refactor of the dashboards implementation, which removes the fragile dependency on blur transforms.

joshuarrrr · 2022-12-09T00:25:12Z

src/themes/oui/oui_colors_dark.scss

+// Breadcrumbs
+$ouiBreadcrumbBlueBackground: #163F66;
+$ouiBreadcrumbGrayBackground: #4C636F;
+$ouiBreadcrumbCollapsedLink: #FFF;
+$ouiBreadCrumbHoverColor: #B0B8BB;


Do we need a todo or issue link here? If memory serves, these colors aren't necessarily intended to be specific to breadcrumbs - they're from an upcoming theme palette. But I may be wrong about that.

cc @KrooshalUX to weigh in on this

joshuarrrr · 2022-12-09T00:27:08Z

src/components/breadcrumbs/breadcrumbs.tsx

+    const isFirstBreadcrumb = index === 0;
    const isLastBreadcrumb = index === breadcrumbs.length - 1;

+    const breadcrumbWrapperClasses = classNames('ouiBreadcrumbWrapper', {
+      'ouiBreadcrumbWrapper--first': isFirstBreadcrumb,
+      'ouiBreadcrumbWrapper--last': isLastBreadcrumb,
+      'ouiBreadcrumbWrapper--truncate': truncate,
+    });


We inherited this - but do we prefer explicitly assigning first/last classes in ts vs just using pseudo selectors in the style files?

src/components/breadcrumbs/breadcrumbs.tsx

joshuarrrr · 2022-12-09T01:17:21Z

src/components/breadcrumbs/_variables.scss

+$ouiBreadcrumbBlueBackground: #B9D9EB !default;
+$ouiBreadcrumbGrayBackground: #D9E1E2 !default;
+$ouiBreadcrumbCollapsedLink: #002A3A !default;
+$ouiBreadCrumbHoverColor: #535861 !default;


If the defaults are set here, do we need to also hardcode the values into the theme?

I believe so, but this is more of an issue with the old theming system, which we are working towards having a cleaner, more pleasant solution for.

Is this related @BSFishy: #212?

If so, left a comment there.

joshuarrrr · 2022-12-09T01:19:28Z

src/components/breadcrumbs/_breadcrumbs.scss

    color: $ouiTextSubduedColor;
+
+    &:hover {
+      color: $ouiBreadCrumbHoverColor !important; // sass-lint:disable-line no-important


Why all the important declarations? We should reconsider the class names and other styles that are causing these conflicts rather than just sledgehammering them.

+1 important should be avoided.

Created an issue for this: #376

joshuarrrr · 2022-12-09T01:23:21Z

src/components/header/__snapshots__/header.test.tsx.snap

+        class="ouiBreadcrumbWrapper ouiBreadcrumbWrapper--first ouiBreadcrumbWrapper--last"
+      >
+        <span
+          aria-current="page"
+          class="ouiBreadcrumb ouiBreadcrumb--last"


Is it necessary to have a --last class on both the wrapper and the breadcrumb? It's OK, just a little redundant and therefore fragile.

KrooshalUX · 2023-02-21T16:03:00Z

@joshuarrrr I am curious if your feedback is blocking for iteration or could those be held until a time where the team can focus on refactoring? The reason I ask is because its blocking release of the component and the documentation in #194 . We can work together to create a separate refactor issue if these pieces of feedback if its non blocking.

* Remove separator Signed-off-by: Matt Provost <provomat@amazon.com> * Fix tests and lints Signed-off-by: Matt Provost <provomat@amazon.com> * Add new breadcrumb style Signed-off-by: Matt Provost <provomat@amazon.com> * Add new style to collapsed breadcrumbs Signed-off-by: Matt Provost <provomat@amazon.com> * Fix some styling issues Signed-off-by: Matt Provost <provomat@amazon.com> * Small cleanup Signed-off-by: Matt Provost <provomat@amazon.com> * Refactor wrapper implementation Signed-off-by: Matt Provost <provomat@amazon.com> * Add breadcrumb wall Signed-off-by: Matt Provost <provomat@amazon.com> * Update some colors Signed-off-by: Matt Provost <provomat@amazon.com> * Fix height issue Signed-off-by: Matt Provost <provomat@amazon.com> --------- Signed-off-by: Matt Provost <provomat@amazon.com> (cherry picked from commit c8942c1)

* Remove separator Signed-off-by: Matt Provost <provomat@amazon.com> * Fix tests and lints Signed-off-by: Matt Provost <provomat@amazon.com> * Add new breadcrumb style Signed-off-by: Matt Provost <provomat@amazon.com> * Add new style to collapsed breadcrumbs Signed-off-by: Matt Provost <provomat@amazon.com> * Fix some styling issues Signed-off-by: Matt Provost <provomat@amazon.com> * Small cleanup Signed-off-by: Matt Provost <provomat@amazon.com> * Refactor wrapper implementation Signed-off-by: Matt Provost <provomat@amazon.com> * Add breadcrumb wall Signed-off-by: Matt Provost <provomat@amazon.com> * Update some colors Signed-off-by: Matt Provost <provomat@amazon.com> * Fix height issue Signed-off-by: Matt Provost <provomat@amazon.com> --------- Signed-off-by: Matt Provost <provomat@amazon.com> (cherry picked from commit c8942c1) Co-authored-by: Matt Provost <provomat@amazon.com> Co-authored-by: Sean Neumann <1413295+seanneumann@users.noreply.github.com>

Signed-off-by: Matt Provost <provomat@amazon.com>

…pensearch-project#379)" This reverts commit 70c12b0. Signed-off-by: Matt Provost <provomat@amazon.com>

BSFishy added 10 commits November 9, 2022 18:17

Remove separator

8d7eab5

Signed-off-by: Matt Provost <provomat@amazon.com>

Fix tests and lints

6d4e7d4

Signed-off-by: Matt Provost <provomat@amazon.com>

Add new breadcrumb style

d5ad006

Signed-off-by: Matt Provost <provomat@amazon.com>

Add new style to collapsed breadcrumbs

774202c

Signed-off-by: Matt Provost <provomat@amazon.com>

Fix some styling issues

f1092e3

Signed-off-by: Matt Provost <provomat@amazon.com>

Small cleanup

e1f13b2

Signed-off-by: Matt Provost <provomat@amazon.com>

Refactor wrapper implementation

4d45c91

Signed-off-by: Matt Provost <provomat@amazon.com>

Add breadcrumb wall

df76e0e

Signed-off-by: Matt Provost <provomat@amazon.com>

Update some colors

bff11ec

Signed-off-by: Matt Provost <provomat@amazon.com>

Fix height issue

ac16dfb

Signed-off-by: Matt Provost <provomat@amazon.com>

BSFishy marked this pull request as ready for review November 21, 2022 18:05

BSFishy requested a review from a team as a code owner November 21, 2022 18:05

BSFishy requested review from joshuarrrr and KrooshalUX November 21, 2022 18:06

BSFishy mentioned this pull request Nov 28, 2022

Breadcrumb Surgical approaches to implement theming #47

Closed

6 tasks

joshuarrrr reviewed Dec 9, 2022

View reviewed changes

BSFishy changed the title ~~Update breadcrumbs~~ Update breadcrumbs to new styling Dec 15, 2022

BSFishy self-assigned this Dec 16, 2022

BSFishy mentioned this pull request Jan 6, 2023

(OUI Docs) Breadcrumbs #194

Open

2 tasks

kavilla linked an issue Feb 21, 2023 that may be closed by this pull request

[UX/OUI] Move global styles header breadcrumb to OUI #22

Closed

kavilla mentioned this pull request Feb 21, 2023

Avoid using important in style sheets #376

Open

kavilla approved these changes Feb 21, 2023

View reviewed changes

joshuarrrr approved these changes Feb 21, 2023

View reviewed changes

joshuarrrr added the backport 1.0 label Feb 21, 2023

joshuarrrr merged commit c8942c1 into opensearch-project:main Feb 21, 2023

opensearch-trigger-bot bot mentioned this pull request Feb 21, 2023

[Backport 1.0] Update breadcrumbs to new styling #379

Merged

BSFishy added a commit to BSFishy/oui that referenced this pull request Apr 13, 2023

Revert "Update breadcrumbs to new styling (opensearch-project#131)"

ba12818

Signed-off-by: Matt Provost <provomat@amazon.com>

BSFishy added a commit that referenced this pull request Apr 14, 2023

Revert "Update breadcrumbs to new styling (#131)" (#706)

166e05c

Signed-off-by: Matt Provost <provomat@amazon.com>

BSFishy added a commit to BSFishy/oui that referenced this pull request May 23, 2023

Revert "Update breadcrumbs to new styling (opensearch-project#131) (o…

0a35580

…pensearch-project#379)" This reverts commit 70c12b0. Signed-off-by: Matt Provost <provomat@amazon.com>

BSFishy deleted the update_breadcrumbs branch July 12, 2023 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update breadcrumbs to new styling #131

Update breadcrumbs to new styling #131

BSFishy commented Nov 21, 2022 •

edited

Loading

AMoo-Miki commented Dec 3, 2022

BSFishy commented Dec 3, 2022

KrooshalUX commented Dec 3, 2022 via email

joshuarrrr commented Dec 5, 2022 •

edited

Loading

joshuarrrr commented Dec 5, 2022 •

edited

Loading

joshuarrrr Dec 9, 2022

BSFishy Dec 9, 2022

kavilla Feb 21, 2023

joshuarrrr Dec 9, 2022

joshuarrrr Dec 9, 2022

BSFishy Dec 9, 2022

kavilla Feb 21, 2023

joshuarrrr Dec 9, 2022

kavilla Feb 21, 2023

kavilla Feb 21, 2023

joshuarrrr Dec 9, 2022

kavilla Feb 21, 2023

KrooshalUX commented Feb 21, 2023

Update breadcrumbs to new styling #131

Update breadcrumbs to new styling #131

Conversation

BSFishy commented Nov 21, 2022 • edited Loading

Description

Issues Resolved

Check List

AMoo-Miki commented Dec 3, 2022

BSFishy commented Dec 3, 2022

KrooshalUX commented Dec 3, 2022 via email

joshuarrrr commented Dec 5, 2022 • edited Loading

joshuarrrr commented Dec 5, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrooshalUX commented Feb 21, 2023

BSFishy commented Nov 21, 2022 •

edited

Loading

joshuarrrr commented Dec 5, 2022 •

edited

Loading

joshuarrrr commented Dec 5, 2022 •

edited

Loading