Tabs updates #4146

tlabaj · 2020-04-28T19:24:47Z

What: Closes #3951

Breaking changes

Tabs:

Changed prop variant for consistency. You will need to update all instances of variant prop to component
Changed TabVariant enum Name to TabComponent for consistent naming. You will now need to update all instances of tabVariant to TabComponent.

Tab:

The title should be wrapped with <TabTitleText> for proper styling. If you would like to place an Icon in the Tab, it should be wrapped with <TabTitleIcon> for proper styling.

patternfly-build · 2020-04-28T19:30:53Z

PF4 preview: https://patternfly-react-pr-4146.surge.sh
A11y report: https://patternfly-react-pr-4146-coverage.surge.sh

dlabrecq · 2020-04-28T20:41:50Z

packages/react-core/src/components/Tabs/Tabs.tsx

+    insetOnMd: undefined,
+    insetOnLg: undefined,
+    insetOnXl: undefined,
+    insetOn2Xl: undefined


Is there a particular reason these props are defined as undefined?

Do you recommend setting it to null? We only want to apply the modifier if one is supplied?

Nit: no reason to set defaults to undefined. Just don't specify a default and they will be undefined.

dlabrecq · 2020-04-28T20:46:11Z

packages/react-core/src/components/Tabs/Tabs.tsx

+    // Update scroll button state and which button to highlight
+    setTimeout(() => {
+      this.handleScrollButtons();
+    }, 1);


We really shouldn't be using setTimeout -- it doesn't always work as expected in production Vs local.

Is there something we can do via componentDidUpdate instead, when state has changed?

Better yet, perhaps use the callback provided by setState?

Why is this needed?

This was preexisting. Not sure why it is here. I am looking into it.

@jschuler this is what I was asking about. I do not understand why we need this.

good question, i did that a year ago :D let's remove it

dlabrecq · 2020-04-29T00:32:27Z

packages/react-core/src/components/Tabs/Tabs.tsx

    }
  };

+  insetString = (s: string) => (s === '2xl' ? '_2xl' : capitalizeFirstLetter(s));


It's not clear what format is expected here. Some comments (i.e., examples) would help clarify how capitalizeFirstLetter is used to build modifiers.

This is an internal function used only by this component would a comment be enough or should I add examples to?

Yes, just looking for code comments here

dlabrecq · 2020-04-29T00:48:40Z

packages/react-core/src/components/Tabs/Tab.tsx

  href?: string;
-  /** Tab title */
+  /** Content rendered in the tab title */
  title: React.ReactNode;


It appears that TabTitleText is required for proper styling. Should the type reflect that?

Or, at least note that in the comments?

Can you also add that to your PR description? Looks like the components to use are not mentioned

mcarrano

@tlabaj I'm comparing this against the Core examples here http://patternfly-v4.surge.sh/documentation/core/components/tabs and I'm not seeing examples for Box tabs. Is this PR intended to reflect all recent tab updates?

tlabaj · 2020-04-29T18:35:20Z

@mcarrano to consolidate the examples, I added a checkbox at the bottom, "isBox" to switch modifier on and off. If it is too confusing, I can separate the examples (that is how I i had it initially).

mcarrano · 2020-04-29T21:06:12Z

I like this approach @tlabaj . It reduces the number of examples which is a good thing :) My only concern is it could be easily missed. Would it be possible to add some text at the top of the examples section to explain this? I can give you some text if you want.

tlabaj · 2020-05-04T17:04:53Z

@mcarrano yes, please provide some text that you think would work.

mcarrano · 2020-05-04T19:35:37Z

@tlabaj how about, "Most tab variations are available as open (default) or box style tabs. Select the 'isBox' checkbox to preview an example with box styled tabs."

redallen

The scrolling behavior seems fine, but maybe we could make a better inset[On(Size)] API more akin to how we do breakpoint mods? Other than that just some nits.

redallen · 2020-05-05T19:05:41Z

packages/react-core/src/components/Tabs/TabTitleIcon.tsx

+
+export const TabTitleIcon: React.FunctionComponent<TabTitleIconProps> = ({
+  children,
+  className = '',


Nit: we don't have to set these to a default of empty string anymore on v4.

I followed the API we have in the drawer component for width.

redallen · 2020-05-05T19:06:44Z

packages/react-core/src/components/Tabs/Tabs.tsx

+    insetOnMd: undefined,
+    insetOnLg: undefined,
+    insetOnXl: undefined,
+    insetOn2Xl: undefined


Nit: no reason to set defaults to undefined. Just don't specify a default and they will be undefined.

redallen · 2020-05-05T19:06:57Z

packages/react-core/src/components/Tabs/Tabs.tsx

+    // Update scroll button state and which button to highlight
+    setTimeout(() => {
+      this.handleScrollButtons();
+    }, 1);


Why is this needed?

redallen · 2020-05-05T19:09:22Z

packages/react-core/src/components/Tabs/Tabs.tsx

+  /** Unmounts tab children (removes them from the DOM) when they are no longer visible */
  unmountOnExit?: boolean;
+  /* Modifies the tabs component padding/inset to visually match padding of other adjacent components.*/
+  inset?: 'none' | 'sm' | 'md' | 'lg' | 'xl' | '2xl';


Maybe something like our breakpoint mods helpers would be a better API here? The structure is looking similar.

redallen · 2020-05-05T19:11:25Z

packages/react-core/src/helpers/util.ts

+ *
+ * @param {string} s string to make first letter a capital
+ */
+export const capitalizeFirstLetter = (s: string) => s.charAt(0).toUpperCase() + s.slice(1);


There's a capitalize helper on line 9 already that does the exact same thing.

I see that now. I just read the description and thought it capitalized the whole string. Thanks! I will update description of original function.

redallen · 2020-05-05T19:18:34Z

packages/react-core/src/components/Tabs/TabTitleText.tsx

+
+export const TabTitleText: React.FunctionComponent<TabTitleTextProps> = ({
+  children,
+  className = '',


Nit: we don't have to set these to a default of empty string anymore on v4.

jschuler · 2020-05-07T13:19:13Z

packages/react-core/src/components/Tabs/TabTitleIcon.tsx

+
+export interface TabTitleIconProps extends React.HTMLProps<HTMLSpanElement> {
+  /** Icon to be rendered inside the tab button title. */
+  children?: React.ReactNode;


i'd make this required

We do not generally make children for similar components required. Is it because the title is required?

without children this component does nothing right? so that's why i think it's not optional. when else should something be required? :)

jschuler · 2020-05-07T13:19:22Z

packages/react-core/src/components/Tabs/TabTitleText.tsx

+
+export interface TabTitleTextProps extends React.HTMLProps<HTMLSpanElement> {
+  /** Text to be rendered inside the tab button title. */
+  children?: React.ReactNode;


this should be required

jschuler · 2020-05-07T13:24:42Z

packages/react-core/src/components/Tabs/Tabs.tsx

+  }
+
+  componentWillUnmount() {
+    document.removeEventListener('resize', this.handleScrollButtons, false);


in componentDidMount you added the listener to window. They both should be the same

Interesting this is how it was before. I did not update here. Should we have it on window on both?

Both should be on window.

jschuler · 2020-05-07T13:25:46Z

packages/react-core/src/components/Tabs/Tabs.tsx

+  formatInsetString = (s: string) => (s === '2xl' ? '_2xl' : capitalize(s));
+
+  componentDidMount() {
+    window.addEventListener('resize', this.handleScrollButtons, false);


Should we only listen to resize if !isVertical? Looks like it needs that condition to add the modifier

jschuler · 2020-05-07T13:27:02Z

packages/react-core/src/components/Tabs/Tabs.tsx

            <AngleLeftIcon />
          </button>
-          <ul className={css(styles.tabsList)} ref={this.tabList}>
+          <ul className={css(styles.tabsList)} ref={this.tabList} onScroll={this.handleScrollButtons}>


similar question here, should we only call the function if ! isVertical?

mattnolting · 2020-05-07T19:27:45Z

Looks like .pf-c-tabs__link is being added twice

mattnolting · 2020-05-07T19:31:52Z

Nav link example scrolls top on click .pf-c-tab__link click https://patternfly-react-pr-4146.surge.sh/documentation/react/components/tabs#using-the-nav-element

codecov-io · 2020-05-08T16:57:09Z

Codecov Report

Merging #4146 into v4 will decrease coverage by 0.12%.
The diff coverage is 64.40%.

@@            Coverage Diff             @@
##               v4    #4146      +/-   ##
==========================================
- Coverage   57.94%   57.82%   -0.13%     
==========================================
  Files         391      393       +2     
  Lines        5982     6010      +28     
  Branches     2354     2364      +10     
==========================================
+ Hits         3466     3475       +9     
- Misses       2066     2091      +25     
+ Partials      450      444       -6

Flag	Coverage Δ
#patternfly4	`57.82% <64.40%> (-0.13%)`	⬇️

Impacted Files	Coverage Δ
...es/react-core/src/components/DataList/DataList.tsx	`90.90% <ø> (ø)`
...-core/src/components/Drawer/DrawerPanelContent.tsx	`45.45% <ø> (ø)`
...eact-core/src/components/FileUpload/FileUpload.tsx	`25.00% <ø> (ø)`
...core/src/components/FileUpload/FileUploadField.tsx	`46.87% <ø> (ø)`
...eact-core/src/components/FormSelect/FormSelect.tsx	`75.00% <ø> (ø)`
...es/react-core/src/components/Nav/NavExpandable.tsx	`51.72% <ø> (ø)`
...rc/components/Pagination/PaginationOptionsMenu.tsx	`78.78% <ø> (ø)`
packages/react-core/src/components/Tabs/Tab.tsx	`100.00% <ø> (ø)`
...kages/react-core/src/components/Tabs/TabButton.tsx	`83.33% <ø> (ø)`
.../react-core/src/components/TextInput/TextInput.tsx	`84.61% <ø> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df0b6ba...5c853b5. Read the comment docs.

jessiehuff

LGTM from an accessibility standpoint! It's keyboard accessible and sounds good in VO. I noticed that axe shows a violation for the aria-controls of two examples which is odd because the axe script didn't flag it, and I can't determine why it was flagged by axe in the browser. It looks unrelated to your PR though so I'll investigate that separately. 🙂

mattnolting

LGTM! 👍

redallen

I'd like to see us use the breakpoint modifier API for insets rather than 5 props. Other than that this LGTM and has great test coverage, thanks @tlabaj !

redallen · 2020-05-11T16:07:36Z

packages/react-core/src/components/Tabs/Tabs.tsx

+  /** Unmounts tab children (removes them from the DOM) when they are no longer visible */
  unmountOnExit?: boolean;
+  /* Modifies the tabs component padding/inset to visually match padding of other adjacent components.*/
+  inset?: 'none' | 'sm' | 'md' | 'lg' | 'xl' | '2xl';


I experimented with our breakpoint modifier API and I think it'd be a good fit and extendable to future modifiers:

export interface TabBreakpointMod { /** The attribute to modify */ modifier: | 'insetNone' | 'insetSm' | 'insetMd' | 'insetLg' | 'insetXl' | 'inset_2xl'; /** The breakpoint at which to apply the modifier */ breakpoint?: 'sm' | 'md' | 'lg' | 'xl' | '2xl'; }

...

/** Array of objects representing the various modifiers to apply to tabs at various breakpoints */ breakpointMods?: TabBreakpointMod[];

...

formatBreakpointMods(breakpointMods, styles),

...

<Tabs activeKey={activeTabKey} onSelect={this.handleTabClick} breakpointMods={[ { breakpoint: 'md', modifier: 'insetSm' }, { breakpoint: 'lg', modifier: 'insetLg' }, { breakpoint: 'xl', modifier: 'inset_2xl' } ]} isBox={isBox}>

Let me know what you think!

Fair enough. I see how this allows for a more flexible interface. But, I do we think if we make this change we should probably go back and update Drawer for consistency.

I agree. I think Drawer should use the same API and we should document it in the breaking change release notes.

Drawer is still beta. Do you mind opening the drawer issue. I will update the Tabs API.

I opened #4238

patternfly#3951

mcoker · 2020-05-11T21:24:24Z

packages/react-core/src/components/Tabs/Tabs.tsx

  rightScrollAriaLabel?: string;
-  /** determines what tag is used around the Tabs. Use "nav" to define the Tabs inside a navigation region */
+  /** Determines what tag is used around the tabs. Use "nav" to define the tabs inside a navigation region */
  variant?: 'div' | 'nav';


do we normally use component where you specify the type of element to be used?

redallen

Need this in for RC1, would be good to change to using the breakpointMods API later.

Comments addressed.

tlabaj added ux review Breaking change 💥 this change requires a major release and has API changes. labels Apr 28, 2020

tlabaj requested review from jessiehuff, jschuler, kmcfaul, mattnolting and mcarrano April 28, 2020 19:24

tlabaj added Breaking change 💥 this change requires a major release and has API changes. and removed Breaking change 💥 this change requires a major release and has API changes. labels Apr 28, 2020

tlabaj changed the title ~~Tab updates~~ Tabs updates Apr 28, 2020

dlabrecq previously requested changes Apr 28, 2020

View reviewed changes

dlabrecq reviewed Apr 29, 2020

View reviewed changes

mcarrano reviewed Apr 29, 2020

View reviewed changes

tlabaj force-pushed the tab_updates branch from 5144df8 to 501dfae Compare May 5, 2020 14:24

redallen suggested changes May 5, 2020

View reviewed changes

jschuler previously requested changes May 7, 2020

View reviewed changes

jessiehuff approved these changes May 8, 2020

View reviewed changes

mattnolting approved these changes May 8, 2020

View reviewed changes

redallen suggested changes May 11, 2020

View reviewed changes

feat(Tabs): Update tabs to match new design

42a1f51

patternfly#3951

Titani added 10 commits May 11, 2020 14:48

update snapshots

edf976c

fix linting errors in md file

c6183a9

updates from code reviews and for issue patternfly#3681

2295dc7

fix linting errors

2b4bdc5

updates from review comments

e9c2c85

updates from review comments

ef9a8d3

remove duplicate class

b2b0b7f

update check for verticle

7605433

update snapshots

64a2057

scroll buttons issue

5eb8490

mcoker reviewed May 11, 2020

View reviewed changes

update the API for inset

0276937

tlabaj force-pushed the tab_updates branch from dfdf0dd to 0276937 Compare May 12, 2020 03:49

Titani added 4 commits May 12, 2020 00:05

change variant prop to component

0261e58

fix linting

bebcb64

fix icon path in demo

cc782b4

update snapshots

5c853b5

redallen approved these changes May 12, 2020

View reviewed changes

redallen merged commit 01afe1d into patternfly:v4 May 12, 2020

This was referenced May 19, 2020

Add tab-rename-variant patternfly/pf-codemods#22

Closed

Add tab-title patternfly/pf-codemods#23

Closed

tlabaj mentioned this pull request Jun 3, 2020

tab key navigation of Tabs does not fully reveal Tab button #3957

Closed

Tabs updates #4146

Tabs updates #4146

Uh oh!

Conversation

tlabaj commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Uh oh!

patternfly-build commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Apr 29, 2020

Uh oh!

mcarrano commented Apr 29, 2020

Uh oh!

tlabaj commented May 4, 2020

Uh oh!

mcarrano commented May 4, 2020

Uh oh!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tlabaj commented Apr 28, 2020 •

edited

Loading

patternfly-build commented Apr 28, 2020 •

edited

Loading

dlabrecq Apr 28, 2020 •

edited

Loading

dlabrecq Apr 29, 2020 •

edited

Loading

codecov-io commented May 8, 2020 •

edited

Loading

redallen May 11, 2020 •

edited

Loading