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: tup-612 isNestedHeader prop in <SectionHeader> #335

Merged
merged 6 commits into from
Nov 14, 2023

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Oct 9, 2023

Overview

Do not nest <header> tags.

Related

Changes

Testing

  1. Open each section.
  2. Inspect elements of the section header.
  3. Verify only one <header> tag is used for the section header.

UI

before after
Projects proj alloc - before proj alloc - after
Project project - before project - after
System sys status - before sys status - after
Unchanged
dashboard account tickets
dashboard - before mng acct - before tickets - before

Notes

Were <Section> used, this prop would not be necessary. But instead <PageLayout> and <SectionHeader> are used independently, allowing developers the freedom to make inconsistent layouts.

I welcome ideas for other solutions (to TUP-612). But, try to avoid suggestions that result in more wrapper elements than the app already (and inconsistently) suffers.

@wesleyboar wesleyboar changed the title feat: isNestedHeader prop in <SectionHeader> feat: tup-612 isNestedHeader prop in <SectionHeader> Oct 9, 2023
Copy link
Contributor

@R-Tomas-Gonzalez R-Tomas-Gonzalez left a comment

Choose a reason for hiding this comment

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

Originally discussed in #339 (comment). The padding-right isn't a side-effect of nested headers. Rather, it's an issue of inconsistent spacing on the sections and articles (specifically padding).

All sections have different classes. Might be helpful to have same class across the board? Not sure, because I don't have any content in my sections.

@wesleyboar
Copy link
Member Author

Might be helpful to have same class across the board

Roight?! I proposed new ticket for solving that, ideally in exactly that way.

@R-Tomas-Gonzalez
Copy link
Contributor

Might be helpful to have same class across the board

Roight?! I proposed new ticket for solving that, ideally in exactly that way.

Right! I agree to making a new ticket. Will approve with branch is updated.

@wesleyboar wesleyboar merged commit 19e785e into main Nov 14, 2023
1 check passed
@wesleyboar wesleyboar deleted the fix/tup-612-nested-header-tags branch November 14, 2023 22:21
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.

2 participants