Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pageheader): new component [khcp-7898] #610

Merged
merged 25 commits into from
Jul 27, 2023

Conversation

kaiarrowood
Copy link
Contributor

@kaiarrowood kaiarrowood commented Jul 13, 2023

Summary

Add new AppPageHeader component for KHCP-7898.
Also adds support to use new design tokens.

Figma: https://www.figma.com/file/8k70HMMdPj98APLaexYJyR/Header?type=design&node-id=11%3A7494&mode=dev

image

PR Checklist

  • Naming & Structure: the files and package structure use the conventions outlined in the Creating a Package docs.
  • Tests pass: check the output of all package unit and/or component tests.
    • If this PR is the result of a bug, test coverage was added accordingly.
  • Functional: all changes do not break existing APIs, but if so, a BREAKING CHANGE commit is in place to bump the major version.
  • Conventional Commits all commits follow the conventional commit standards outlined in the main README.
  • Docs: includes a technically accurate README, and the docs have been updated accordingly based on the changes in this PR.

@kaiarrowood kaiarrowood self-assigned this Jul 13, 2023
@kaiarrowood kaiarrowood force-pushed the feat/khcp-7898-page-header branch from 7d1f8b4 to 4bd3386 Compare July 13, 2023 14:43
@kaiarrowood kaiarrowood changed the title Feat/khcp 7898 page header feat(pageheader): new component [khcp-7898] Jul 13, 2023
@kaiarrowood kaiarrowood force-pushed the feat/khcp-7898-page-header branch from 29b8b06 to c739ca3 Compare July 24, 2023 21:16
@kaiarrowood kaiarrowood marked this pull request as ready for review July 25, 2023 16:01
@kaiarrowood kaiarrowood requested a review from a team July 25, 2023 16:02
package.json Outdated Show resolved Hide resolved
packages/core/app-layout/docs/page-header.md Show resolved Hide resolved
packages/core/app-layout/docs/page-header.md Outdated Show resolved Hide resolved
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

A couple things I noticed that seem like they need to be addressed in the UI:

  • The spacing between the title and the badge seems too close when compared to the Figma
    image

  • This component doesn't appear to be ideally responsive below certain viewport sizes. Can you please add some media queries (using the kui-breakpoint-* tokens for media queries) and tweak the layout a bit so that the items properly stack on mobile?

  • If the title text is too long, it send the action menu off of the page. I think we need to add a flex container around the title and action menu (or whatever the right-side content is called) so that the title can wrap to a new line and the action menu remains visible on the right
    image

@kaiarrowood kaiarrowood force-pushed the feat/khcp-7898-page-header branch from d1a30aa to fe95809 Compare July 25, 2023 22:10
@kaiarrowood kaiarrowood reopened this Jul 25, 2023
@kaiarrowood
Copy link
Contributor Author

A couple things I noticed that seem like they need to be addressed in the UI:

  • The spacing between the title and the badge seems too close when compared to the Figma
    image

Fixed

  • This component doesn't appear to be ideally responsive below certain viewport sizes. Can you please add some media queries (using the kui-breakpoint-* tokens for media queries) and tweak the layout a bit so that the items properly stack on mobile?

Added mobile breakpoint styles

  • If the title text is too long, it send the action menu off of the page. I think we need to add a flex container around the title and action menu (or whatever the right-side content is called) so that the title can wrap to a new line and the action menu remains visible on the right
    image

Truncating the title in that scenario instead as agreed by design. Thanks for the help fixing this one @portikM !

@adamdehaven ready for re-review

@kaiarrowood kaiarrowood merged commit 863891c into main Jul 27, 2023
@kaiarrowood kaiarrowood deleted the feat/khcp-7898-page-header branch July 27, 2023 13:42
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.

5 participants