chore(Tabs): Tabs table demo by gabipodolnikova · Pull Request #6692 · patternfly/patternfly-react

gabipodolnikova · 2021-12-10T13:24:40Z

What: Closes #6514

With the current configuration the examples needs to be used as js isFullscreen file="./examples/Tabs/TabsAndTable.tsx". I tried it as a ts file, but the docs build is failing.
Also to fully use TS in demos, it needs to be configured, which I was not able to do. In the end, I had to keep the DashboardWrapper and DashboardHeader in JS :(

patternfly-build · 2021-12-10T13:28:54Z

Preview: https://patternfly-react-pr-6692.surge.sh

nicolethoen · 2021-12-20T20:23:58Z

packages/tsconfig.base.json

    "**/**.test.ts",
-    "**/examples/**"
+    "**/examples/**",
+    "**/demos/Tabs/**"


What purpose does adding this line serve? What error do you see if you didn't add it?

oh yeah, nothing now... removing... I moved the TS file to examples and forgot about this

mcarrano

This looks good @gabipodolnikova . The only question I have is why there are subtle differences in the layout between this and the core demo here: https://www.patternfly.org/v4/components/tabs/html-demos/tables-and-tabs/# They have nothing to do with the tabs, but seems like the mastheads are different? Not sure which is more correct. Can you take a look @mcoker ?

mcoker · 2021-12-20T23:31:15Z

@mcarrano the react demo is using the masthead component, and the core demo is using the pre-existing page header in the page component. Looks like these are the places that still use the page header and not the masthead in core:

src/patternfly//demos/Drawer/drawer-demo-default.hbs
src/patternfly//demos/Page/page-demo-expandable-nav.hbs
src/patternfly//demos/Page/page-template-header.hbs
src/patternfly//demos/Alert/examples/page-default.hbs
src/patternfly//demos/Skeleton/examples/Skeleton.md
src/patternfly//demos/DataList/data-list-page-header.hbs
src/patternfly//demos/Wizard/examples/Wizard.md
src/patternfly//demos/Table/table-page-header.hbs
src/patternfly//demos/PrimaryDetail/primary-detail-template.hbs
src/patternfly//demos/Nav/examples/Nav.md
src/patternfly//demos/NotificationDrawer/notification-drawer-demo.hbs
src/patternfly//demos/CardView/examples/CardView.md
src/patternfly//demos/Banner/__banner-page-header.hbs

mcarrano

@mcoker I opened a new core issue here to get those updated: patternfly/patternfly#4579. I think this one is good to go then so I will approve it. Thanks @gabipodolnikova !

wise-king-sullyman

LGTM other than this one nit.

wise-king-sullyman · 2021-12-23T20:11:43Z

packages/react-core/src/demos/Tabs.md

+```
+
+### Tables and tabs
+```js isFullscreen file="./examples/Tabs/TabsAndTable.tsx"


Suggested change

```js isFullscreen file="./examples/Tabs/TabsAndTable.tsx"

```ts isFullscreen file="./examples/Tabs/TabsAndTable.tsx"

As I said in the PR description, it is not possible with the current config to import it like you suggested... :(

wise-king-sullyman

Changing to approve and spinning up a separate issue for my previous comment since it seems that the crux of the problem lies with the markdown parser.

mcoker

The only bug I found is that clicking on either an action column kebab or an item in the kebab dropdown will trigger row selection.

Here are some other difference between the core and react demos. wdyt of these @mcarrano?

Core has divider between toolbar and table, react doesn't have divider
Core has full/non-compact bottom pagination, react uses compact
Core using space-items-lg in drawer panel with content, progress, labels, etc, react uses default spacing (medium)
Core uses outline variation of labels in drawer panel, react uses default
Core uses 33% drawer panel width, react uses default
Core doesn't have breadcrumbs
Core doesn't use width-limited page sections - react does.

mcarrano · 2022-01-04T22:00:50Z

@mcoker Thanks for highlighting all of the differences between the demos. I knew they looked different but I couldn't put my finger on why.

I'm kind of on the fence here. None of these issues really have bearing on the problem at hand, but I can see people being confused about why these are different. In general I think of the core demos as defining the desired layout/appearance and the React demo just adding interaction. But maybe that's oversimplified.

@nicolethoen @tlabaj @mceledonia @mmenestr what do you think? Do you think it's important to have a direct visual match between equivalent core and React demos?

mmenestr · 2022-01-05T16:29:47Z

Not sure where to look to find both the core and react demos to compare, but l personally think it's important to keep the visuals the same. When designers are looking at them, I don't think they'd think "focus on the interaction for react, and take the visuals from core" even though that's really the aim. They'd probably just think "why are the visuals different in core and react and which one is correct."

In general, the more we can keep things consistent and avoid small differences, the better. That way we can avoid general designer confusion as to how a component should look/be implemented!

mcarrano · 2022-01-05T19:28:29Z

Thanks for this input @mmenestr I agree with your main points.

As I look at this more, I can see that there are both inconsistencies between core and React and also inconsistencies between the core demos themselves. For example, if you look at all the core demos for tabs, some have a breadcrumb and others do not. What would help here? Should we have a common template for all full page demos that includes common elements like the masthead, navigation, breadcrumbs, etc? Could that be used as a starter for all full page demos to make sure these inconsistencies don't creep in? It's true that even though these details seem trivial, they can lead to inconsistent implementations in product.

I welcome your thoughts on how to fix this. As for this specific issue, @gabipodolnikova can you clean up the current demo to address some of the obvious differences (like the toolbar and pagination) and we can address the larger problem as a separate issue, I think?

mcoker · 2022-01-05T19:32:12Z

Should we have a common template for all full page demos that includes common elements like the masthead, navigation, breadcrumbs, etc? Could that be used as a starter for all full page demos to make sure these inconsistencies don't creep in? It's true that even though these details seem trivial, they can lead to inconsistent implementations in product.

@mcarrano yep, we have templates like this we can use in any component/demo example, and we have page demo templates per component demo (like a common page template demo we use for all table demos, or data list demos, for example). So we should definitely re-visit this and make sure we're using a common template. Would you mind opning a core issue?

mcarrano · 2022-01-05T22:01:57Z

I've opened a new core issue here: patternfly/patternfly#4586 We can talk more about what should be in these templates and how to ensure they get used when creating new demos.

gabipodolnikova · 2022-01-06T14:02:41Z

@mcarrano @mcoker So I covered the mentioned differences between core and react and fixed the issue with the kebab toggle.
There are still some differences, but they are also in the other demos too as we also use a templates (for the Navigation and Masthead)...

mcarrano · 2022-01-06T15:18:46Z

Thanks @gabipodolnikova . Looks good to me!

mcoker

LGTM!

tlabaj

Looks great Gabi!

patternfly-build · 2022-01-06T21:33:35Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.22.4
@patternfly/react-catalog-view-extension@4.34.4
@patternfly/react-charts@6.36.4
@patternfly/react-code-editor@4.24.4
@patternfly/react-console@4.34.4
@patternfly/react-core@4.183.4
@patternfly/react-docs@5.44.4
@patternfly/react-icons@4.34.4
@patternfly/react-inline-edit-extension@4.28.4
demo-app-ts@4.143.4
@patternfly/react-integration@4.145.4
@patternfly/react-log-viewer@4.28.4
@patternfly/react-styles@4.33.4
@patternfly/react-table@4.52.4
@patternfly/react-tokens@4.35.4
@patternfly/react-topology@4.30.4
@patternfly/react-virtualized-extension@4.30.4
transformer-cjs-imports@4.21.4

Thanks for your contribution! 🎉

gabipodolnikova added 4 commits December 10, 2021 15:48

chore(Tabs): Added tabs with table demo, wip

aeb2544

Adds drawer panel content

5c27b7a

chore(Tabs): TabsAndTable demo form js to ts

217031a

updates test snapshot

9c533da

gabipodolnikova force-pushed the tabs-table-demo branch from cb280a5 to 9c533da Compare December 20, 2021 09:15

fixes the failing docs build

98b7ab6

gabipodolnikova marked this pull request as ready for review December 20, 2021 12:39

nicolethoen reviewed Dec 20, 2021

View reviewed changes

nicolethoen requested review from mcarrano, tlabaj and wise-king-sullyman December 20, 2021 20:24

mcarrano reviewed Dec 20, 2021

View reviewed changes

mcarrano approved these changes Dec 21, 2021

View reviewed changes

removing forgotten line in config

9d7b54c

nicolethoen approved these changes Dec 22, 2021

View reviewed changes

wise-king-sullyman requested changes Jan 4, 2022

View reviewed changes

gabipodolnikova requested a review from wise-king-sullyman January 4, 2022 15:28

wise-king-sullyman approved these changes Jan 4, 2022

View reviewed changes

tlabaj added the css review label Jan 4, 2022

tlabaj requested a review from mcoker January 4, 2022 17:10

wise-king-sullyman mentioned this pull request May 10, 2022

Markdown parser unable to process some TypeScript demos when ts code blocks are used patternfly/patternfly-org#2958

Closed

mcoker requested changes Jan 4, 2022

View reviewed changes

Review css fixes

d1a5972

gabipodolnikova requested a review from mcoker January 6, 2022 14:02

mcoker approved these changes Jan 6, 2022

View reviewed changes

tlabaj approved these changes Jan 6, 2022

View reviewed changes

tlabaj merged commit 2915155 into patternfly:main Jan 6, 2022

	```js isFullscreen file="./examples/Tabs/TabsAndTable.tsx"
	```ts isFullscreen file="./examples/Tabs/TabsAndTable.tsx"

Conversation

gabipodolnikova commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolethoen Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

gabipodolnikova Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker commented Dec 20, 2021

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

gabipodolnikova Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment • 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!

mcarrano commented Jan 4, 2022

Uh oh!

mmenestr commented Jan 5, 2022

Uh oh!

mcarrano commented Jan 5, 2022

Uh oh!

mcoker commented Jan 5, 2022

Uh oh!

mcarrano commented Jan 5, 2022

Uh oh!

gabipodolnikova commented Jan 6, 2022

Uh oh!

mcarrano commented Jan 6, 2022

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Jan 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gabipodolnikova commented Dec 10, 2021 •

edited

Loading

patternfly-build commented Dec 10, 2021 •

edited

Loading

mcoker left a comment •

edited

Loading