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

New main content header component #34

Merged
9 commits merged into from
Jun 8, 2020
Merged

New main content header component #34

9 commits merged into from
Jun 8, 2020

Conversation

gsusmi
Copy link
Collaborator

@gsusmi gsusmi commented Jun 2, 2020

main-content-header

Main content header component contains a title(basic & dropdown) with
actions. Title dropdown is styled Rose::Header::Dropdown component whereas header actions are simple Rose::Button components.

<Rose::Layout::Main::Header as |header|>
  <header.title as |title|>
    <title.dropdown @text="Page title" @icon="user-square-fill" as |dropdown|>
      <dropdown.link @route="index">Menu item</dropdown.link>
      <dropdown.link @route="about">Menu item</dropdown.link>
      <dropdown.button>Button menu item</dropdown.button>
    </title.dropdown>
  </header.title>
  <header.action @style="primary">Primary</header.action>
  <header.action @style="primary">Secondary</header.action>
</Rose::Layout::Main::Header>

@gsusmi gsusmi requested a review from a user June 2, 2020 13:34
title=(component 'rose/layout/main/header/title')
)}}

<div class="rose-layout-main-header-actions">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wrapper div as providing a new class name through attributes isn't merged into button classname correctly.
Ref: emberjs/ember.js#17533

@gsusmi
Copy link
Collaborator Author

gsusmi commented Jun 2, 2020

Updates:
Discussions in slack led to evolving layout component pattern.
Introduced new Rose::Layout::PageHeader component who's sole purpose is to define grid for content to be placed into.
Introduced new Rose::PageHeader::Title and Rose::PageHeader::Utilities components to contain content for page header.

I was able to refactor previous use of dropdown title component, so I'd like to leave it as it is if non-detrimental.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Let's ditch the dedicated page header layout stuff and build a page/content layout with four regions:

  • header
  • actions/utilities
  • navigation
  • main/body
<Rose::Layout::Page as |page|>
  <page.header>
    <h2>Title</h2>
  </page.header>
  <page.actions>
    <button>stuff</button>
  </page.actions>
  <page.navigation>
    <tabs />
  </page.navigation>
  <page.body>
    <p>content</p>
  </page.body>
</Rose::Layout::Page>

The more I think about it, we can probably get away with using simple heading tags for the title <h2> for now, until we have more design detail.

And rename the existing "page" layout to Global to align with Structure: Rose::Layout::Global.

@ghost
Copy link

ghost commented Jun 4, 2020

For now, don't worry about the link button styles. We can come back to that soon. Let's get the layouts settled.

@gsusmi gsusmi force-pushed the ICU-189-main-header branch from 062a7ac to c4023ab Compare June 8, 2020 01:21
@gsusmi
Copy link
Collaborator Author

gsusmi commented Jun 8, 2020

Updates:
Split out layouts into Global and Page.

<Rose::Layout::Global as |global|>
  <global.header />
  <global.navigation />
  <global.body />
</Rose::Layout::Global>

Page layout is to be placed into <Rose::Layout::Global::Body>

<Rose::Layout::Page as |page|>
  <page.breadcrumbs />
  <page.header />
  <page.actions />
  <page.navigation />
  <page.body />
</Rose::Layout::Page>

Rose::Layout::PageHeader has been removed, however, it's content components are still available under Rose::PageHeader::Title and Rose::PageHeader::Utilities.

@gsusmi gsusmi requested a review from a user June 8, 2020 01:47
@gsusmi
Copy link
Collaborator Author

gsusmi commented Jun 8, 2020

@randallmorey , regarding minimizing layout sass files, in one of our previous discussions/PRs, you voiced interest in expanding sass files to replicate associated style used in templates. Should I not be following this pattern anymore? I'm always happy with less files to search through.

@ghost
Copy link

ghost commented Jun 8, 2020

The layout regions are all essentially identical. I created a ticket to consolidate these. Going forward it doesn't seem worthwhile to split them into their own files.

@ghost ghost merged commit 0ecccb6 into develop Jun 8, 2020
@ghost ghost deleted the ICU-189-main-header branch June 8, 2020 14:48
ghost pushed a commit that referenced this pull request Jun 14, 2023
…6f56f6d42f5bea0f31d0786a

[main] OSS to ENT merge of (0cecb0d)
This pull request was closed.
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