PF4: feat(Tabs) Add Tabs for PF4 #1144

tlabaj · 2019-01-07T22:13:03Z

What:#926

Additional issues:

patternfly-build · 2019-01-07T22:30:26Z

PatternFly-React preview: https://1144-pr-patternfly-react-patternfly.surge.sh

coveralls · 2019-01-07T22:31:21Z

Pull Request Test Coverage Report for Build 4187

64 of 67 (95.52%) changed or added relevant lines in 4 files are covered.
7 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.07%) to 80.144%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/internal/util.js	20	21	95.24%
packages/patternfly-4/react-core/src/components/Tabs/Tabs.js	38	40	95.0%

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-4/react-core/src/components/Chip/Chip.js	7	33.33%

Totals
Change from base Build 4179:	0.07%
Covered Lines:	4622
Relevant Lines:	5414

💛 - Coveralls

rvsia · 2019-01-08T13:47:56Z

packages/patternfly-4/react-core/src/components/index.ts

 export * from './Text';
 export * from './TextArea';
 export * from './TextInput';
 export * from './Title';
 export * from './Tooltip';
+export * from './TextArea';


Duplicates here

mcarrano

@tlabaj This is looking good but there are two issues I see.

Secondary tabs should be styled differently from primary tabs. See the core example that shows the correct secondary tab styling.
When the tabs do not fit horizontally, I don't believe we should be displaying a horizontal scroll bar. The purpose of adding the arrows to the end of the tab bar was to eliminate the need to scroll. @mceledonia can you confirm?

andybraren · 2019-01-09T17:20:44Z

I've been chatting off-site with Titani, the secondary tab styling fix is in progress (was missing the .pf-m-tabs-secondary modifier) and the scrollbar issue that Christie mentioned over in Core's PR 1068 will be fixed once React pulls that in. HTML/CSS looks good to me once those are done. 👍

@mcarrano Regarding the presence of scrollbars, based on my note in Core Issue 85 I think we agreed at the end of our meeting to keep scrollbars and remove the original scrollbar-hiding hack to avoid fighting with browsers (primarily Firefox). Soon after that discussion Firefox made it possible to fully hide scrollbars (covered in Core Issue 1103) but doing so will still negatively impact users who explicitly ask for scrollbars to be shown in their browser's settings.

andybraren

Once the latest Core CSS changes are added and the .pf-m-tabs-secondary modifier is added to secondary tabs the HTML/CSS looks good to me. Nice job Titani, especially with the fancy scroll button behavior! 🙂

mcarrano · 2019-01-09T21:35:19Z

I do recall that discussion now, @andybraren . @mceledonia what do you think about the scroll bar?

mcarrano

Secondary tabs are now correct. I'm OK to move forward.

andybraren · 2019-01-15T18:37:02Z

The CSS is almost there - it just needs the refactoring and tweaks included in Core's PR #1068 to fix things like the scroll button bug that Christie noted here.

tlabaj · 2019-01-16T14:56:16Z

@andybraren Workspace so it pulls in the updated css. Please try the latest preview. Make sure to clear your cache first.

andybraren

HTML/CSS looks good, thank you for pulling in those latest changes! CSS approved.

From an IXD perspective, I notice two things:

The scroll buttons in the "Secondary buttons" example seem susceptible to breaking, particularly the ones for secondary tabs. Sometimes the regular "Scroll buttons" example breaks too, but less often. I'm not quite sure why this is. Chrome's Console doesn't report anything breaking.
The scroll button "math" seems to position the left-most pixel of the next tab directly underneath the left-most pixel of the scroll button, covering the first part of the tab's label (see below, "Tab" is covered). Could that math be tweaked to position the tab's left-most pixel directly to the right of the left scroll button's right-most pixel?

mcarrano · 2019-01-16T15:54:30Z

Now I see what you are saying about the scroll breaking @andybraren . I have to admit that this is pretty ugly from an IxD perspective, especially when there are two rows of tabs. @mceledonia have you looked at this? Is there possibly another way to handle this?

The original intent was mainly to provide a solution for tabs on a mobile device. Has anyone tested it there? I wonder how well that works. In most desktop scenarios we should not see this unless there are an extreme number of tabs.

andybraren · 2019-01-16T16:45:54Z

The original design linked at the top of Core PR #868 didn't include an example of scrolling secondary tabs. Here's a video of what they look like on iOS 12; better but not perfect.

If scrollbars on desktop are the main "ugly" part, that circles back to the discussion in Core Issue #859.

Also notice in the video above that two taps are required to "click" a tab - the first one just activates the hover state. This could be solved either via CSS or JS. I'll bring this up within the CSS group to figure out which would be most appropriate.

christiemolloy

CSS looks good here. I know there's some discussion about what will happen with scroll bars and arrows, but for the meantime this is consistent with Core

mcarrano · 2019-01-18T22:22:05Z

Secondary tabs look good now. But why to tabs show with a gray background now?

andybraren

(One sec, checking on the grey background)

andybraren

One last thing, the grey background issue is fixed in Core 1246, could you include that change?

jgiardino · 2019-01-23T10:54:24Z

I created issue patternfly/patternfly#1127 to provide consistent labels for <nav> elements. For the .pf-c-tabs element, this should be customizable by the consumer to be either <div class="pf-c-tabs"> or <nav class="pf-c-tabs" aria-label="Local">.

<nav> with aria-label would be used when there is a unique set of tabs on a page, so that screen reader users can find this navigation when browsing by landmark region.

Is this possible with this implementation? Is this something that should be captured in a separate issue?

christiemolloy

Nice work!!!

andybraren · 2019-01-28T17:24:55Z

@jgiardino That makes sense to me - I created Core #1328 to track that change.

tlabaj · 2019-01-28T18:25:29Z

@mcarrano @andybraren Please take another look. The PR to updated to the version of core with the background fix went in this morning.

tlabaj · 2019-01-28T18:27:01Z

@jgiardino if you are ok with it. I think we should open a separate issue so we don't hold this PR up.

andybraren

With the latest Core changes the grey background issue is now gone. Great job Titani! 🎉

tlabaj · 2019-01-28T19:36:11Z

@andy @jgiardino I added issue #1268 to track the accessibility issue.

ibolton336 · 2019-01-28T20:15:07Z

packages/patternfly-4/react-core/src/components/Tabs/Tab.d.ts

+import { SFC, HTMLProps } from 'react';
+
+export interface TabProps extends HTMLProps<HTMLDivElement> {
+  children: any;


We don't need children as that is included in html props.

I removed this.

ibolton336 · 2019-01-28T20:15:36Z

packages/patternfly-4/react-core/src/components/Tabs/Tabs.d.ts

+import { Omit } from '../../typeUtils';
+
+export interface TabsProps extends Omit<HTMLProps<HTMLDivElement>, 'onSelect'> {
+  children: any;


children included in html props

don't I need to override . it here since it is required?

You should import this and add this to the typescript integration app since it's more complicated component.

ibolton336 · 2019-01-28T20:17:08Z

packages/patternfly-4/react-core/src/components/Tabs/Tabs.js

+  static propTypes = propTypes;
+  static defaultProps = defaultProps;
+
+  constructor(props) {


Since this is a class component & we are using the babel class properties plugin, we don't need a constructor. You can add the createref right on the class like tablist= React.createRef()

ibolton336 · 2019-01-28T20:18:27Z

packages/patternfly-4/react-core/src/components/Tabs/Tabs.js

+            styles.tabs,
+            isFilled && styles.modifiers.fill,
+            isSecondary && styles.modifiers.tabsSecondary,
+            this.state.showLeftScrollButton && styles.modifiers.start,


can we destructure these state values like we do for the props?

good idea. thanks.

This reverts commit ca890d9.

dlabaj · 2019-01-29T16:46:49Z

packages/patternfly-4/react-core/src/components/Tabs/Tabs.d.ts

+import { Omit } from '../../typeUtils';
+
+export interface TabsProps extends Omit<HTMLProps<HTMLDivElement>, 'onSelect'> {
+  children: any;


You should import this and add this to the typescript integration app since it's more complicated component.

tlabaj requested review from jschuler, dlabaj, christiemolloy and mceledonia January 7, 2019 22:13

tlabaj added ux review css review PF4 React issues for the PF4 core effort labels Jan 7, 2019

rvsia reviewed Jan 8, 2019

View reviewed changes

tlabaj requested a review from andybraren January 8, 2019 19:39

mcarrano requested changes Jan 8, 2019

View reviewed changes

andybraren previously approved these changes Jan 9, 2019

View reviewed changes

tlabaj dismissed andybraren’s stale review via 2f1938a January 9, 2019 17:28

tlabaj force-pushed the tabs branch from e73735d to 2f1938a Compare January 9, 2019 17:28

tlabaj added the enhancement 🚀 label Jan 11, 2019

mcarrano previously approved these changes Jan 15, 2019

View reviewed changes

tlabaj dismissed mcarrano’s stale review via f173c9b January 15, 2019 21:09

tlabaj force-pushed the tabs branch from b6101de to f173c9b Compare January 15, 2019 21:09

andybraren previously approved these changes Jan 16, 2019

View reviewed changes

tlabaj dismissed andybraren’s stale review via ca890d9 January 16, 2019 20:29

tlabaj force-pushed the tabs branch from 1d259d4 to ca890d9 Compare January 16, 2019 20:29

christiemolloy previously approved these changes Jan 17, 2019

View reviewed changes

tlabaj dismissed christiemolloy’s stale review via 48ee04f January 18, 2019 21:17

andybraren approved these changes Jan 18, 2019

View reviewed changes

andybraren requested changes Jan 18, 2019

View reviewed changes

jgiardino mentioned this pull request Jan 23, 2019

Consistent labels for <nav> elements patternfly/patternfly#1127

Closed

christiemolloy previously approved these changes Jan 28, 2019

View reviewed changes

andybraren mentioned this pull request Jan 28, 2019

Tabs: add example of local navigational tabs patternfly/patternfly#1328

Closed

andybraren previously approved these changes Jan 28, 2019

View reviewed changes

ibolton336 suggested changes Jan 28, 2019

View reviewed changes

tlabaj added 9 commits January 29, 2019 11:00

PF4: feat(Tabs) Add Tabs for PF4

4330d35

update based on reviews

2a80a6c

update examples to match core

e5e9cee

update test snapshot

b0b306b

add missing index.d.ts file

6f8bc54

Remove secondary tabs

70f1a55

Revert "Remove secondary tabs"

23b12ad

This reverts commit ca890d9.

remove scroll buttons from secondary tabs

ca69701

updates based on review comments

826bca5

tlabaj dismissed stale reviews from andybraren and christiemolloy via 826bca5 January 29, 2019 16:10

tlabaj force-pushed the tabs branch from f9be3b0 to 826bca5 Compare January 29, 2019 16:10

Add tabs to integration App

a1cdf9c

dlabaj approved these changes Jan 29, 2019

View reviewed changes

ibolton336 approved these changes Jan 29, 2019

View reviewed changes

dlabaj merged commit 2a505a8 into patternfly:master Jan 29, 2019

tlabaj mentioned this pull request Jan 29, 2019

PF4 Tabs #926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PF4: feat(Tabs) Add Tabs for PF4 #1144

PF4: feat(Tabs) Add Tabs for PF4 #1144

tlabaj commented Jan 7, 2019

patternfly-build commented Jan 7, 2019

coveralls commented Jan 7, 2019 •

edited

rvsia Jan 8, 2019

mcarrano left a comment

andybraren commented Jan 9, 2019

andybraren left a comment

mcarrano commented Jan 9, 2019

mcarrano left a comment

andybraren commented Jan 15, 2019

tlabaj commented Jan 16, 2019

andybraren left a comment

mcarrano commented Jan 16, 2019

andybraren commented Jan 16, 2019

christiemolloy left a comment

mcarrano commented Jan 18, 2019

andybraren left a comment •

edited

andybraren left a comment

jgiardino commented Jan 23, 2019

christiemolloy left a comment

andybraren commented Jan 28, 2019

tlabaj commented Jan 28, 2019

tlabaj commented Jan 28, 2019

andybraren left a comment

tlabaj commented Jan 28, 2019

ibolton336 Jan 28, 2019

tlabaj Jan 29, 2019

ibolton336 Jan 28, 2019

tlabaj Jan 29, 2019

dlabaj Jan 29, 2019

ibolton336 Jan 28, 2019

ibolton336 Jan 28, 2019

tlabaj Jan 29, 2019

dlabaj Jan 29, 2019

PF4: feat(Tabs) Add Tabs for PF4 #1144

PF4: feat(Tabs) Add Tabs for PF4 #1144

Conversation

tlabaj commented Jan 7, 2019

patternfly-build commented Jan 7, 2019

coveralls commented Jan 7, 2019 • edited

Pull Request Test Coverage Report for Build 4187

💛 - Coveralls

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

andybraren commented Jan 9, 2019

andybraren left a comment

Choose a reason for hiding this comment

mcarrano commented Jan 9, 2019

mcarrano left a comment

Choose a reason for hiding this comment

andybraren commented Jan 15, 2019

tlabaj commented Jan 16, 2019

andybraren left a comment

Choose a reason for hiding this comment

mcarrano commented Jan 16, 2019

andybraren commented Jan 16, 2019

christiemolloy left a comment

Choose a reason for hiding this comment

mcarrano commented Jan 18, 2019

andybraren left a comment • edited

Choose a reason for hiding this comment

andybraren left a comment

Choose a reason for hiding this comment

jgiardino commented Jan 23, 2019

christiemolloy left a comment

Choose a reason for hiding this comment

andybraren commented Jan 28, 2019

tlabaj commented Jan 28, 2019

tlabaj commented Jan 28, 2019

andybraren left a comment

Choose a reason for hiding this comment

tlabaj commented Jan 28, 2019

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

coveralls commented Jan 7, 2019 •

edited

andybraren left a comment •

edited