fix: Tabs: Initial clean up, get rtl and ltr consistent #5431

margaree · 2025-03-11T16:12:21Z

The purpose of this PR is to get the LTR and RTL styles consistent in order to make reading the vdiffs easier with the changes to use the new tab structure/component

github-actions · 2025-03-11T16:13:28Z

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL	https://live.d2l.dev/prs/BrightspaceUI/core/pr-5431/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

margaree · 2025-03-11T16:13:08Z

components/tabs/tab-internal.js

-				margin-left: 0;
-			}
-			:host([dir="rtl"]:first-child) .d2l-tab-text {
-				margin-left: 0.6rem;


margin-right for first child for LTR is actually 0.5rem so this was inconsistent in RTL. Let me know if there was a reason why that I'm missing.

Can we remove RtlMixin entirely from some of these are is the dir attribute still needed for something?

Ah I see Dave already asked that elsewhere -- ignore this one.

margaree · 2025-03-11T16:13:59Z

components/tabs/tab-internal.js

 				width: calc(100% - 0.6rem);
 			}
-			:host([dir="rtl"]:first-child) .d2l-tab-selected-indicator {
-				margin-left: 0.6rem;


This is handled in the css for d2l-tab-selected-indicator above.

margaree · 2025-03-11T16:14:32Z

components/tabs/tab-mixin.js

@@ -37,7 +37,6 @@ export const TabMixin = superclass => class extends SkeletonMixin(superclass) {
 				width: calc(100% - 1.2rem);
 			}
 			:host(:first-child) .d2l-tab-selected-indicator {
-				margin-inline-end: 0.6rem;


This wasn't needed

margaree · 2025-03-11T16:15:38Z

components/tabs/tab.js

@@ -15,7 +15,6 @@ class Tab extends TabMixin(LitElement) {
 				white-space: nowrap;
 			}
 			:host(:first-child) .d2l-tab-text {
-				margin-inline-end: 0.6rem;


This wasn't yet used but was inconsistent with the current LTR behaviour (which would have margin-inline-end as 0.5rem)

margaree · 2025-03-11T16:16:24Z

components/tabs/tabs.js

@@ -19,34 +19,6 @@ const reduceMotion = matchMedia('(prefers-reduced-motion: reduce)').matches;

 const scrollButtonWidth = 56;

-// remove once IE11 is no longer supported
-if (!Array.prototype.findIndex) {


no longer needed

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

dbatiste · 2025-03-11T16:38:14Z

If we want to go so far as to eliminate the use of RtlMixin, I think we would need to check document.documentElement.getAttribute('dir') === 'rtl' in the Javascript, similar to what I did for the PopoverMixin.

bearfriend · 2025-03-11T17:07:27Z

Only skimmed this but if it's relevant to this or future changes remember we have --d2l-document-direction if needed

margaree · 2025-03-11T17:30:34Z

If we want to go so far as to eliminate the use of RtlMixin, I think we would need to check document.documentElement.getAttribute('dir') === 'rtl' in the Javascript, similar to what I did for the PopoverMixin.

We check for dir in a variety of places in the js in tabs.js. In each spot I can just do a document.documentElement.getAttribute('dir') !== / === 'rtl'. Instead of that, is there a spot we'd recommend for putting a property? Or should we still be using RtlMixin so a re-render gets triggers if dir changes?

dbatiste · 2025-03-11T18:12:39Z

Instead of that, is there a spot we'd recommend for putting a property? Or should we still be using RtlMixin so a re-render gets triggers if dir changes?

I'm fine with doing the getAttribute(...) call for each of the few functions that need it. One of the functions contains 4 usages of dir that would obviously be replaced by just one call.

But if we want to store the value in a shared spot to reduce the calls, we'd just want to do it in a spot such that it gets updated when we change the language in our demo pages. I'm not convinced this is worth it.

I might be inclined to put the document.documentElement.getAttribute('dir') === 'rtl', in a isRTL method though.

…ceUI/core into tabs-consistent-styles-cleanup

…if rtl

d2l-github-release-tokens · 2025-03-11T19:19:05Z

🎉 This PR is included in version 3.88.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: Tabs: Initial clean up, get rtl and ltr consistent

3c7bc9e

margaree commented Mar 11, 2025

View reviewed changes

github-actions bot mentioned this pull request Mar 11, 2025

Updating vdiff goldens for PR 5431 #5432

Merged

margaree marked this pull request as ready for review March 11, 2025 16:23

margaree requested a review from a team as a code owner March 11, 2025 16:23

dbatiste approved these changes Mar 11, 2025

View reviewed changes

Updating vdiff goldens (#5432)

8dcf18e

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

code review feedback: remove rtlmixin in tab-internal

245d825

margaree added 2 commits March 11, 2025 14:20

Merge branch 'tabs-consistent-styles-cleanup' of github.com:Brightspa…

4fd6438

…ceUI/core into tabs-consistent-styles-cleanup

code review feedback: remove RTLMixin, add helper method for getting …

4e4c07b

…if rtl

dbatiste approved these changes Mar 11, 2025

View reviewed changes

margaree merged commit ab02660 into main Mar 11, 2025
7 checks passed

margaree deleted the tabs-consistent-styles-cleanup branch March 11, 2025 19:16

d2l-github-release-tokens bot added the released label Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Tabs: Initial clean up, get rtl and ltr consistent #5431

fix: Tabs: Initial clean up, get rtl and ltr consistent #5431

margaree commented Mar 11, 2025

github-actions bot commented Mar 11, 2025

margaree Mar 11, 2025 •

edited

Loading

dlockhart Mar 11, 2025

dlockhart Mar 11, 2025

margaree Mar 11, 2025

margaree Mar 11, 2025

margaree Mar 11, 2025

margaree Mar 11, 2025

dbatiste commented Mar 11, 2025

bearfriend commented Mar 11, 2025 •

edited

Loading

margaree commented Mar 11, 2025

dbatiste commented Mar 11, 2025 •

edited

Loading

d2l-github-release-tokens bot commented Mar 11, 2025

fix: Tabs: Initial clean up, get rtl and ltr consistent #5431

fix: Tabs: Initial clean up, get rtl and ltr consistent #5431

Conversation

margaree commented Mar 11, 2025

github-actions bot commented Mar 11, 2025

margaree Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

dlockhart Mar 11, 2025

Choose a reason for hiding this comment

dlockhart Mar 11, 2025

Choose a reason for hiding this comment

margaree Mar 11, 2025

Choose a reason for hiding this comment

margaree Mar 11, 2025

Choose a reason for hiding this comment

margaree Mar 11, 2025

Choose a reason for hiding this comment

margaree Mar 11, 2025

Choose a reason for hiding this comment

dbatiste commented Mar 11, 2025

bearfriend commented Mar 11, 2025 • edited Loading

margaree commented Mar 11, 2025

dbatiste commented Mar 11, 2025 • edited Loading

d2l-github-release-tokens bot commented Mar 11, 2025

margaree Mar 11, 2025 •

edited

Loading

bearfriend commented Mar 11, 2025 •

edited

Loading

dbatiste commented Mar 11, 2025 •

edited

Loading