Skip to content

Conversation

@aferd
Copy link
Collaborator

@aferd aferd commented May 28, 2024

@aferd aferd marked this pull request as draft May 28, 2024 16:50
@patternfly-build
Copy link

patternfly-build commented May 28, 2024

@aferd aferd marked this pull request as ready for review May 29, 2024 18:03
@fhlavac
Copy link
Contributor

fhlavac commented May 29, 2024

@aferd can you please add cypress component tests for the core functionality?

@fhlavac
Copy link
Contributor

fhlavac commented May 30, 2024

I've tried to summarize my comments here, it should also help with the layout issues you've mentioned.

import React from 'react';
import {
  Divider,
  Flex,
  FlexItem,
  Split,
  SplitItem,
  Text,
  PageSection,
  TextContent,
  Button,
  ButtonVariant,
  ButtonProps
} from '@patternfly/react-core';
import { ExternalLinkAltIcon } from '@patternfly/react-icons';

export interface PageHeaderLinkProps extends ButtonProps {
  /** Title for the link */
  label: string;
  /** Indicates if the link points to an external page */
  isExternal?: boolean;
}

export interface ContentHeaderProps {
  /** Title for content header */
  title: string;
  /** Subtitle for content header */
  subtitle: string;
  /** Optional link below subtitle */
  linkProps?: PageHeaderLinkProps;
  /** Optional icon for content header (appears to the left of the content header's title with a divider) */
  icon?: React.ReactNode;
  /** Optional badge for content header (appears to the right of the content header's title) */
  label?: React.ReactNode;
  /** Breadcrumbs component */
  breadcrumbs?: React.ReactNode;
  /** Menu that appears to the far right of the title */
  actionMenu?: React.ReactNode;
  /** Custom OUIA ID */
  ouiaId?: string | number;
}

export const ContentHeader: React.FunctionComponent<React.PropsWithChildren<ContentHeaderProps>> = ({
  title,
  subtitle,
  linkProps,
  icon,
  label,
  breadcrumbs = null,
  actionMenu,
}: ContentHeaderProps) => (
  <PageSection variant="light" className='pf-v5-u-p-md'>
    <div className="pf-v5-u-mb-md">
      {breadcrumbs}
    </div>
    <Flex>
      {icon && (
        <>
          <FlexItem>
            {icon}
          </FlexItem>
          <Divider orientation={{ default: 'vertical' }} />
        </>
      )}
      <FlexItem flex={{ default: 'flex_1' }}>
        <Split hasGutter>
          <SplitItem>
            <TextContent>
              <Text component="h1">
                {title}
              </Text>
            </TextContent>
          </SplitItem>
          {label && (
            <SplitItem>
              {label}
            </SplitItem>
          )}
          <SplitItem isFilled />
          {actionMenu && (
            <SplitItem>
              {actionMenu}
            </SplitItem>
          )}
        </Split>
        <TextContent>
          <Text component="p">
            {subtitle}
          </Text>
          {linkProps && (
            <Button variant={ButtonVariant.link} isInline icon={linkProps.isExternal ? <ExternalLinkAltIcon /> : null} iconPosition="end" {...linkProps}>
              {linkProps.label}
            </Button>
          )}
        </TextContent>
      </FlexItem>
    </Flex>
  </PageSection>
);

export default ContentHeader;

@fhlavac
Copy link
Contributor

fhlavac commented May 30, 2024

@aferd one last thing I've missed. Could you please also add OUIA ids to the key component's elements similarly to other components in this repo? It should also help with the cypress testing. Thank you 🙂

@aferd aferd requested a review from fhlavac June 4, 2024 17:29
@fhlavac
Copy link
Contributor

fhlavac commented Jun 5, 2024

@aferd the a11y tests seem to be failing on missing image-alt

@fhlavac
Copy link
Contributor

fhlavac commented Jun 5, 2024

According to the PF example, the title should have a 8px bottom margin
image
https://www.patternfly.org/components/page/react-demos/sticky-section-group/

@fhlavac
Copy link
Contributor

fhlavac commented Jun 5, 2024

@aferd left some more comments, but it looks great! Thank you 🎉

@fhlavac
Copy link
Contributor

fhlavac commented Jun 6, 2024

@aferd just two small comments to improve the examples. Looks great! Let's wait for the UX review.
Can you also squash the changes to 2-3 meaningful commits? Also feat(ContentHeader): prefix should be added to the commit message to trigger a release after merge.

@andrew-ronaldson
Copy link

I noticed the label next to the title seemed to be aligned to the top of the text. Think that flex layout could align center?

@fhlavac
Copy link
Contributor

fhlavac commented Jun 7, 2024

@aferd perfect, if you could clean up the commits a bit, and this one might be good to go! 🎉

@andrew-ronaldson
Copy link

Last update from me, sorry!
The content header with icon. Can we center that vertically with the title area. I added align-self:center but not sure if there is an existing pf modifier for that.
Screenshot 2024-06-07 at 9 37 20 AM
Inspected code
Screenshot 2024-06-07 at 9 51 29 AM

Sorry for this one too but can you remove the pf-v5-u-align-content-centerfrom the div around the label. What you have makes the text align on the baseline with the title, which is nice, but the overall label feels top aligned.

Screenshot 2024-06-07 at 9 52 09 AM

Copy link

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaaaaaaaaproved. thanks!

Copy link
Contributor

@fhlavac fhlavac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks @aferd 🎉

@fhlavac fhlavac merged commit db776ce into patternfly:main Jun 7, 2024
nicolethoen pushed a commit to fhlavac/react-component-groups that referenced this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants