refactor(tabs, tab, tab-nav, tab-title): `getElementProp` is refactored out in favor of inheritable props set directly on parent #7605

Elijbet · 2023-08-25T00:44:06Z

Related Issue: #6038

Summary

getElementProp is refactored out across child components as an outdated pattern in favor of inheritable props set directly on the parent. Instead of the tab, tab-title, tab-nav looking up the parent for position and scale, these get set by the tabs parent.

The logic for setting these props thus moves to the parent, getting rid of the getElementProp altogether. The parent component gets a mutationObserver to achieve this and watchers for when it needs to modify the children. Child attribute selectors are subbed with new classes added dynamically to reflect the scale and position, as attributes are no longer reflected to be targeted.

Inherited props addressed:

position
scale

… out in favor of inheritable props set directly on parent

…eflected

…ed on children

github-actions · 2023-09-21T01:43:43Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

…ed scale class in the format .scale--s

…osition and other cleanup

jcfranco

👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾
👾🕹️🕹️🕹️🕹️👾🕹️👾👾👾👾👾🕹️👾🕹️🕹️🕹️🕹️👾👾🕹️🕹️🕹️👾👾🕹️🕹️👾👾🕹️👾👾👾🕹️👾🕹️🕹️🕹️🕹️👾🕹️👾
👾🕹️👾👾🕹️👾🕹️👾👾👾👾👾🕹️👾🕹️👾👾👾👾🕹️👾👾👾👾🕹️👾👾🕹️👾🕹️🕹️👾🕹️🕹️👾🕹️👾👾👾👾🕹️👾
👾🕹️🕹️🕹️🕹️👾🕹️👾👾🕹️👾👾🕹️👾🕹️🕹️🕹️👾👾👾🕹️🕹️👾👾🕹️👾👾🕹️👾🕹️👾🕹️👾🕹️👾🕹️🕹️🕹️👾👾🕹️👾
👾🕹️👾👾🕹️👾🕹️👾🕹️👾🕹️👾🕹️👾🕹️👾👾👾👾👾👾👾🕹️👾🕹️👾👾🕹️👾🕹️👾👾👾🕹️👾🕹️👾👾👾👾👾👾
👾🕹️👾👾🕹️👾👾🕹️👾👾👾🕹️👾👾🕹️🕹️🕹️🕹️👾🕹️🕹️🕹️👾👾👾🕹️🕹️👾👾🕹️👾👾👾🕹️👾🕹️🕹️🕹️🕹️👾🕹️👾
👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾👾

packages/calcite-components/src/components/tab/tab.scss

packages/calcite-components/src/components/tabs/tabs.tsx

packages/calcite-components/src/components/tab/tab.e2e.ts

…bute

packages/calcite-components/src/components/tab-nav/tab-nav.scss

jcfranco · 2023-10-30T16:26:26Z

packages/calcite-components/src/components/tab/tab.tsx

@@ -69,7 +69,7 @@ export class Tab {
          role="tabpanel"
          tabIndex={this.selected ? 0 : -1}
        >
-          <section>
+          <section class={CSS.content}>


Not for this PR, but it'd be good to replace all selectors using section with the content class.

jcfranco · 2023-10-30T16:27:03Z

packages/calcite-components/src/components/tabs/tabs.stories.ts

-    position="${select("position", ["top", "bottom"], "top")}"
-    scale="${select("scale", ["s", "m", "l"], "l")}"
-  >
+const TabNavHTMLSimple = html`


Awesome job adding more coverage! ✨🧪🧪🧪✨

jcfranco · 2023-10-30T16:27:35Z

packages/calcite-components/src/components/tabs/tabs.tsx

@@ -166,7 +166,7 @@ export class Tabs {
    }
  });

-  private updateItems = (): void => {
+  private updateItems(): void {


jcfranco · 2023-10-30T16:29:11Z

packages/calcite-components/src/components/tabs/tabs.tsx

+      }
+    );
+
+    Array.from(this.el.querySelectorAll("calcite-tab")).forEach((tab: HTMLCalciteTabElement) => {


Queries target all descendants, so this will be an issue when you have nested tabs with different props each. Not sure how common position and scale being different between nested tabs would be, so this could be an edge case.

…herited props to other nested tab-titles

@jcfranco

**Related Issue:** N/A ## Summary Fails on unrelated PR: #7605 Looks like it's DST-related, as London is used in E2E tests and it had a DST adjustment on 10/2 per @jcfranco 🫤 🤔 🤨 🧐

Elijbet added 2 commits August 24, 2023 15:08

refactor(tabs, tab, tab-nav, tab-title): getElementProp is refactored…

cefb472

… out in favor of inheritable props set directly on parent

include tab scale

9f05d24

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Aug 25, 2023

github-actions bot assigned Elijbet Aug 25, 2023

Elijbet changed the title ~~refactor(tabs, tab, tab-nav, tab-title): getElementProp is refactored out in favor of inheritable props set directly on parent~~ refactor(tabs, tab, tab-nav, tab-title): getElementProp is refactored out in favor of inheritable props set directly on parent Aug 25, 2023

Elijbet added 17 commits August 30, 2023 16:32

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

7f5a322

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

3f704d3

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

44db41f

update tests per new changes

27c8c25

adjust tab-nav tests to asserting on scales being inherited and not r…

b322908

…eflected

make testing scale inheritance down the ancestry tree dry

4b572a4

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

c106a94

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

c64c5fc

update tabs test per new requirements: attribute is no longer reflect…

c5fa9bf

…ed on children

cleanup

85c5b5d

cleanup

88df173

clean loop with absent value

4fba6d2

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

6fd166b

refactor selectors out into resources, other refactors

41fefb6

Refactor tab-nav scale test into one compact it block

07d83bf

add a class selector and refactor test loop

c17919e

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

e9a4969

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Sep 21, 2023

Elijbet added 6 commits October 12, 2023 13:03

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

c1500e8

refactor use of attribute scale as selector and use a dynamically add…

25529f7

…ed scale class in the format .scale--s

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

5d9254e

cleanup tabs

e115f45

cleanup

cf561c7

use a css selector instead of an attribute selector in the tabs for p…

336ea70

…osition and other cleanup

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 26, 2023

jcfranco approved these changes Oct 27, 2023

View reviewed changes

packages/calcite-components/src/components/tab/tab.scss Outdated Show resolved Hide resolved

packages/calcite-components/src/components/tabs/tabs.tsx Outdated Show resolved Hide resolved

packages/calcite-components/src/components/tab/tab.e2e.ts Show resolved Hide resolved

Elijbet added 6 commits October 27, 2023 14:45

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

37b877c

add screenshot coverage for simle center/inline/border scale variations

d4e508e

cleanup

c1a9c9d

make these styles more robust by using a class vs an element

d2b10fc

set of screenshot tests to cover various scenarios for scales

9288052

use a class method vs an assigned-bound function

468787e

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 28, 2023

rework margin application based on new scale class selectors vs attri…

6aa6c87

…bute

Elijbet removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 29, 2023

Elijbet mentioned this pull request Oct 29, 2023

[Tabs]: Refactor the mutationObserver to invoke when nested children are added/removed #8078

Open

4 tasks

Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 29, 2023

geospatialem added the low risk Issues with low risk for consideration in low risk milestones label Oct 30, 2023

jcfranco reviewed Oct 30, 2023

View reviewed changes

Elijbet added 3 commits October 30, 2023 10:59

fix to missing inline tab gap and make sure tabs doesn't push it's in…

52ff51c

…herited props to other nested tab-titles

fix omission

ff3defb

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

3184404

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 30, 2023

Elijbet mentioned this pull request Oct 30, 2023

test: skip unstable tests #8085

Merged

Merge branch 'main' into elijbet/6038-refactor-getElementProp-tab-title

f065e26

Elijbet added skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 30, 2023

Elijbet merged commit e070d7d into main Oct 30, 2023
14 checks passed

Elijbet deleted the elijbet/6038-refactor-getElementProp-tab-title branch October 30, 2023 19:43

github-actions bot added this to the 2023 October Priorities - Maintenance Release milestone Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tabs, tab, tab-nav, tab-title): `getElementProp` is refactored out in favor of inheritable props set directly on parent #7605

refactor(tabs, tab, tab-nav, tab-title): `getElementProp` is refactored out in favor of inheritable props set directly on parent #7605

Elijbet commented Aug 25, 2023 •

edited

Loading

github-actions bot commented Sep 21, 2023

jcfranco left a comment

jcfranco Oct 30, 2023

jcfranco Oct 30, 2023

jcfranco Oct 30, 2023

jcfranco Oct 30, 2023

refactor(tabs, tab, tab-nav, tab-title): getElementProp is refactored out in favor of inheritable props set directly on parent #7605

refactor(tabs, tab, tab-nav, tab-title): getElementProp is refactored out in favor of inheritable props set directly on parent #7605

Conversation

Elijbet commented Aug 25, 2023 • edited Loading

Summary

github-actions bot commented Sep 21, 2023

jcfranco left a comment

Choose a reason for hiding this comment

jcfranco Oct 30, 2023

Choose a reason for hiding this comment

jcfranco Oct 30, 2023

Choose a reason for hiding this comment

jcfranco Oct 30, 2023

Choose a reason for hiding this comment

jcfranco Oct 30, 2023

Choose a reason for hiding this comment

refactor(tabs, tab, tab-nav, tab-title): `getElementProp` is refactored out in favor of inheritable props set directly on parent #7605

refactor(tabs, tab, tab-nav, tab-title): `getElementProp` is refactored out in favor of inheritable props set directly on parent #7605

Elijbet commented Aug 25, 2023 •

edited

Loading