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(about-section): new component [khcp-8589] #782

Merged
merged 18 commits into from
Sep 15, 2023

Conversation

kaiarrowood
Copy link
Contributor

@kaiarrowood kaiarrowood commented Sep 13, 2023

Summary

New AppAboutSection component for KHCP-8589.
Also minor fixes for TopNTable:

  • Fix KCard styles
  • Add isLoading prop

Figma: https://www.figma.com/file/8UBIeLOY3YX3oF0RtDdwau/About-Section?type=design&node-id=55%3A9902&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 Sep 13, 2023
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.

The responsiveness overall looks great, but one issue with the created/modified timestamps

image

Can we drop these to a new line, under the title or description, on smaller screens?

Comment on lines 89 to 100
createdLabel: {
type: String,
default: '',
},
modified: {
type: String,
default: '',
},
modifiedLabel: {
type: String,
default: '',
},
Copy link
Member

Choose a reason for hiding this comment

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

question: why would the createdLabel or modifiedLabel ever be different text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For User/Developer pages we are going to display Joined instead of Created

Copy link
Member

Choose a reason for hiding this comment

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

If that's the case, should the prop names for created/modified be more generic?

e.g. createdLabel: 'Joined' seems a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a suggestion, it maps to created_at/updated_at fields most of the time.

@adamdehaven
Copy link
Member

image

I’m worried a bit about the amount of content being displayed at the bottom of the component. Is this accounted for in the responsive style rules?

Obviously mobile is going to be a stretch, but even smaller laptop screens are going to have to wrap/stack some of this content.

Could you update the sandbox to see what this looks like and then we can inspect on smaller screens?

@kaiarrowood kaiarrowood marked this pull request as ready for review September 14, 2023 19:44
@kaiarrowood kaiarrowood requested review from a team and jillztom as code owners September 14, 2023 19:44
@kaiarrowood kaiarrowood enabled auto-merge (squash) September 15, 2023 13:26
@adamdehaven adamdehaven merged commit d975afb into main Sep 15, 2023
6 checks passed
@adamdehaven adamdehaven deleted the feat/khcp-8589-about-section branch September 15, 2023 13:29
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.

3 participants