feat(Page): updated logic to allow section grouping #10650

thatblindgeye · 2024-06-25T15:40:51Z

What: Closes #10622

Codemod issue: patternfly/pf-codemods#674

Additional issues:

patternfly-build · 2024-06-25T15:44:56Z

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

A11y report: https://patternfly-react-pr-10650-a11y.surge.sh

wise-king-sullyman

🔥

mcoker

Left a few comments. I'm also curious what the difference is between:

This hardcoded styles.pageMainBreadcrumb in Page.tsx
<PageBreadcrumb>
<PageSection type="breadcrumb">

Just curious if we need all 3, and if so, if we could just have one that generates the markup/styles and the other two places could use it (so we're only creating that section markup once)

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

mcoker · 2024-07-03T01:21:00Z

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

@@ -287,7 +288,7 @@ class Page extends React.Component<PageProps, PageState> {
          )
        )}
      >
-        {isBreadcrumbWidthLimited ? <div className={css(styles.pageMainBody)}>{breadcrumb}</div> : breadcrumb}
+        {isBreadcrumbWidthLimited ? <PageMainBody>{breadcrumb}</PageMainBody> : breadcrumb}


Since <PageMainBody> should always wrap the children of all page section (__main-group), was there a reason it's only being added here with isBreadcrumbWidthLimited?

Though looking at this branch, looks like you're on core alpha.150 but the core changes to handle the changes to allow for multiple bodies are in alpha.162 - can you bump core in this PR?

Bumped to latest Core, this will fail due to other PRs needing to go in first (Switch, JumpLinks, etc).

packages/react-core/src/components/Page/PageMainBody.tsx

mcoker · 2024-07-03T01:25:21Z

packages/react-core/src/components/Page/PageSection.tsx

@@ -127,8 +131,7 @@ export const PageSection: React.FunctionComponent<PageSectionProps> = ({
      {...(hasOverflowScroll && { tabIndex: 0 })}
      aria-label={ariaLabel}
    >
-      {isWidthLimited && <div className={css(styles.pageMainBody)}>{children}</div>}
-      {!isWidthLimited && children}
+      {hasMainBodyWrapper ? <PageMainBody>{children}</PageMainBody> : children}


mcoker · 2024-07-03T01:30:28Z

packages/react-core/src/components/Page/PageMainBody.tsx

+  </div>
+);
+
+PageMainBody.displayName = 'PageMainBody';


Just a nit but all of the other sections are <PageBreadcrumb>, <PageGroup>, <PageSection> - should we call this <PageBody>?

thatblindgeye · 2024-07-03T18:08:35Z

@mcoker

Just curious if we need all 3, and if so, if we could just have one that generates the markup/styles and the other two places could use it (so we're only creating that section markup once)

Updated the first instance. For PageSection with type breadcrumb, do we still intend for a PageSection component to have the breadcrumb class applied? Or should consumers just use the PageBreadcrumb component instead? There are some shared classes being applied to both, PageSection having more that can be applied. Alternatively I suppose we axe PageBreadcrumb and just use PageSection with the type="breadcrumb" passed in.

mcoker · 2024-07-03T19:45:16Z

or PageSection with type breadcrumb, do we still intend for a PageSection component to have the breadcrumb class applied?

@thatblindgeye yep - as far as I can tell those 3 places added the same class for the breadcrumb section, which we will want to keep. From my perspective, I'd say whatever works better from your perspective re: which to keep - <PageBreadcrumb> or <PageSection type="breadcrumb">. Also don't want to add any overhead to this PR, so if it isn't something we want to tackle now, totally fine, I was mostly just curious why there were 3.

thatblindgeye · 2024-07-08T12:27:42Z

@mcoker Maybe a post-beta thing to look into regarding the PageBreadcrumb component vs PageSection with breadcrumb variant modifier? Would depend if we want to deprecate PageBreadcrumb/if there'd be any repercussions found when looking into it further.

mcoker

Just one little thing. Also wanted to note there is a yarn.lock change committed, but no package.json changes - want to make sure that's intentional?

mcoker · 2024-07-15T17:19:08Z

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

      nav = (
-        <div className={css(styles.pageMainNav, styles.modifiers.limitWidth)}>
-          <div className={css(styles.pageMainBody)}>{horizontalSubnav}</div>
+        <div className={css(styles.pageMainSubnav, styles.modifiers.limitWidth)}>
+          <PageBody>{horizontalSubnav}</PageBody>
        </div>
      );
    } else if (horizontalSubnav) {
-      nav = <div className={css(styles.pageMainNav)}>{horizontalSubnav}</div>;
+      nav = <div className={css(styles.pageMainSubnav)}>{horizontalSubnav}</div>;
    }


Since <PageBody> should wrap the children of all sections except for page__main-group, I think you could update this so <PageBody> is there with and without isHorizontalSubnavWidthLimited, and we just dynamically render styles.modifiers.limitWidth based on whether that prop is true or false.

mcoker

🍯🐝 🐻

patternfly-build · 2024-07-15T18:55:24Z

Your changes have been released in:

@patternfly/react-code-editor@6.0.0-alpha.83
@patternfly/react-core@6.0.0-alpha.83
@patternfly/react-docs@7.0.0-alpha.90
@patternfly/react-drag-drop@6.0.0-alpha.65
@patternfly/react-integration@6.0.0-alpha.43
demo-app-ts@5.1.1-alpha.82
@patternfly/react-table@6.0.0-alpha.84
@patternfly/react-templates@6.0.0-alpha.33

Thanks for your contribution! 🎉

thatblindgeye requested review from tlabaj, wise-king-sullyman and mcoker June 25, 2024 15:40

thatblindgeye added css review require codemod labels Jun 25, 2024

thatblindgeye mentioned this pull request Jun 25, 2024

Page - updated logic for rendering PageMainBody patternfly/pf-codemods#674

Closed

thatblindgeye linked an issue Jun 25, 2024 that may be closed by this pull request

Page - updates to allow for section grouping #10622

Closed

wise-king-sullyman approved these changes Jul 2, 2024

View reviewed changes

mcoker reviewed Jul 3, 2024

View reviewed changes

thatblindgeye force-pushed the iss10622_pageUpdates branch from 980971b to bd8ac18 Compare July 3, 2024 18:04

thatblindgeye force-pushed the iss10622_pageUpdates branch from bd8ac18 to c29243f Compare July 8, 2024 12:26

thatblindgeye force-pushed the iss10622_pageUpdates branch from c786551 to 5b8efdd Compare July 15, 2024 12:47

mcoker reviewed Jul 15, 2024

View reviewed changes

thatblindgeye added 9 commits July 15, 2024 13:28

feat(Page): updated logic to allow section grouping

0bfe09d

Updated and added unit tests

a365aaa

Added snapshot test

7c57c3c

Removed PageNavigation component

39dda52

PR feedback

f71ac95

Rebased, updated yarn lock

02ec197

Updated snapshots

caa77f2

Renamed test file

c56467f

Updated logic to always render PageBody for page nav

12d2327

thatblindgeye force-pushed the iss10622_pageUpdates branch from 81f2410 to 12d2327 Compare July 15, 2024 17:32

mcoker approved these changes Jul 15, 2024

View reviewed changes

Updated tests

68b1c32

dlabaj approved these changes Jul 15, 2024

View reviewed changes

dlabaj merged commit b9181ae into patternfly:v6 Jul 15, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Page): updated logic to allow section grouping #10650

feat(Page): updated logic to allow section grouping #10650

thatblindgeye commented Jun 25, 2024 •

edited

Loading

patternfly-build commented Jun 25, 2024 •

edited

Loading

wise-king-sullyman left a comment

mcoker left a comment

mcoker Jul 3, 2024

thatblindgeye Jul 3, 2024

mcoker Jul 3, 2024

mcoker Jul 3, 2024

thatblindgeye commented Jul 3, 2024

mcoker commented Jul 3, 2024

thatblindgeye commented Jul 8, 2024

mcoker left a comment

mcoker Jul 15, 2024

mcoker left a comment

patternfly-build commented Jul 15, 2024

feat(Page): updated logic to allow section grouping #10650

feat(Page): updated logic to allow section grouping #10650

Conversation

thatblindgeye commented Jun 25, 2024 • edited Loading

patternfly-build commented Jun 25, 2024 • edited Loading

wise-king-sullyman left a comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

mcoker Jul 3, 2024

Choose a reason for hiding this comment

thatblindgeye Jul 3, 2024

Choose a reason for hiding this comment

mcoker Jul 3, 2024

Choose a reason for hiding this comment

mcoker Jul 3, 2024

Choose a reason for hiding this comment

thatblindgeye commented Jul 3, 2024

mcoker commented Jul 3, 2024

thatblindgeye commented Jul 8, 2024

mcoker left a comment

Choose a reason for hiding this comment

mcoker Jul 15, 2024

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

patternfly-build commented Jul 15, 2024

thatblindgeye commented Jun 25, 2024 •

edited

Loading

patternfly-build commented Jun 25, 2024 •

edited

Loading