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(ui5-dynamic-page): introduce new component #7899

Merged
merged 59 commits into from
May 29, 2024
Merged

feat(ui5-dynamic-page): introduce new component #7899

merged 59 commits into from
May 29, 2024

Conversation

dobrinyonkov
Copy link
Contributor

@dobrinyonkov dobrinyonkov commented Nov 22, 2023

New ui5-dynamic-page component.

The component is a composition of the following subcomponents:

  • DynamicPageTitle - holds the most top area of DynamicPage (breadcrumbs, actions, etc.)
  • DynamicPageHeader - a container displayed between the title area and the content that allows generic contet.
  • DynamicPageHeaderActions (private) - abstraction of the expand and pin header buttons, allowing for better reusability when state is expanded or collpased.

dobrinyonkov and others added 22 commits September 28, 2023 15:14
…#7845)

* feat(ui5-dynamic-page): ensure visibility of title content that never overflows

* feat(ui5-dynamic-page) actions style corrected

Co-authored-by: Dobrin Dimchev <[email protected]>

* feat(ui5-dynamic-page): sizing of toolbar wrappers corrected

* feat(ui5-dynamic-page): apply review feedback

---------

Co-authored-by: Dobrin Dimchev <[email protected]>
* feat(ui5-dynamic-page): prevent unnecessary content overflow when footer hidden

* feat(ui5-dynamic-page): move scroll listener to the new scroll container
* feat(ui5-dynamic-page): fit content that has 100% height

* feat(ui5-dynamic-page): correct import

* feat(ui5-dynamic-page): add tests
* feat(ui5-dynamic-page): keyboard handling

* feat(ui5-dynamic-page): add focus span

* fix: add expand and focus on title click

---------

Co-authored-by: Dobrin Dimchev <[email protected]>
fix(ui5-dynamic-page): apply theme styles

Co-authored-by: PetyaMarkovaBogdanova <[email protected]>
* feat(ui5-dynamic-page): screen reader support

* feat(ui5-dynamic-page): add tests
fix(ui5-dynamic-page): story and docs added

Co-authored-by: PetyaMarkovaBogdanova <[email protected]>
@ilhan007 ilhan007 added this to the 2.0.0 milestone Apr 30, 2024
Copy link
Contributor

@nnaydenow nnaydenow left a comment

Choose a reason for hiding this comment

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

  • Expand / collapse, pin buttons are visible even if header is not used
  • Title is missing visual separator (shadow) when header doesn't exist
  • If the header is snapped while the user is at the top of the page, it can't be expanded via scroll (deviation with openui5)
  • If the user starts to scroll from the position where header is snapped (basically around 0-5 top) and scroll a bit up the whole header appear.
  • Hovering expand / collapse button doesn't change ui5-dynamic-page-title background color
  • action button appear over content on mobile
    image

packages/fiori/src/DynamicPage.ts Outdated Show resolved Hide resolved

prepareLayoutActions() {
// all navigation/layout actions should have the NeverOverflow behavior
const navigationActions = this.querySelector<Toolbar>("[ui5-toolbar][slot='navigationActions']");
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to use querySelector here instead of this.navigationActions. In the documentation of the slot it's not described that we expect ui5-toolbar so if we do it we can describe the slot as:

	@slot({ type: HTMLElement })
	navigationActions!: Array<Toolbar>;

Copy link
Contributor

Choose a reason for hiding this comment

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

If you expect toolbar to be assigned to actions / navigationActions it would be good to rename them to navigationBar/actionsBar otherwise it will be confused because I would assume that i have to pass ui5-button instead of ui5-toolbar

@nnaydenow
Copy link
Contributor

I was thinking if it’s a good idea to provide parts for the main areas since they have responsive paddings by default

@ilhan007
Copy link
Member

ilhan007 commented May 13, 2024

provide parts for the main areas

We had such requirements for the TabContainer in the past - it also implements responsive paddings, but still consumers wanted to clear them for some reason. So I think - it's ok to include them.

* @public
*/
@slot({ "default": true, type: HTMLElement })
content!: HTMLElement[];
Copy link
Member

Choose a reason for hiding this comment

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

How we use content, expandedContent and snappedContent, isn't only 2 states expended and snapped and two slots enough

/**
* Defines the content of the Heading of the Dynamic Page.
*
* @public
Copy link
Member

Choose a reason for hiding this comment

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

How we use heading, expendedHeading, snappedHeading, isn't only 2 states expended and snapped and two slots enough

@@ -0,0 +1,8 @@
import Basic from "../../../_samples/fiori/DynamicPage/Basic/Basic.md";

Copy link
Contributor

Choose a reason for hiding this comment

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

add slug in frontmatter format for all dynamic page components


prepareLayoutActions() {
// all navigation/layout actions should have the NeverOverflow behavior
const navigationActions = this.querySelector<Toolbar>("[ui5-toolbar][slot='navigationActions']");
Copy link
Contributor

Choose a reason for hiding this comment

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

If you expect toolbar to be assigned to actions / navigationActions it would be good to rename them to navigationBar/actionsBar otherwise it will be confused because I would assume that i have to pass ui5-button instead of ui5-toolbar

@nnaydenow
Copy link
Contributor

nnaydenow commented May 27, 2024

I found 2 more issues:

  • Wrong visual appearance of navigation bar when ui5-dynamic-page doesn't have breadcrumbs:
<ui5-dynamic-page show-footer>
    <ui5-dynamic-page-title slot="titleArea">
        <ui5-title slot="heading">...</ui5-title>
        <div slot="snappedHeading">...</div>
        <p slot="content">...</p>
        <p slot="snappedContent">...</p>
        <ui5-tag color-scheme="7">...</ui5-tag>
        <ui5-toolbar slot="actionsBar">...</ui5-toolbar>
        <ui5-toolbar slot="navigationBar">
            <ui5-toolbar-button design="Transparent" icon="share"></ui5-toolbar-button>
            <ui5-toolbar-button design="Transparent" icon="action-settings"></ui5-toolbar-button>
        </ui5-toolbar>
    </ui5-dynamic-page-title>
    <ui5-dynamic-page-header slot="headerArea">
        ...
    </ui5-dynamic-page-header>
    <ui5-list>
        ...
    </ui5-list>
    <ui5-bar slot="footerArea" design="FloatingFooter">
        ...
    </ui5-bar>
</ui5-dynamic-page>

image

  • ui5-dynamic-page-title is clickable/toggleable when ui5-dynamic-page-header doesn't exist in ui5-dynamic-page
<ui5-dynamic-page show-footer>
    <ui5-dynamic-page-title slot="titleArea">
        <ui5-title slot="heading">...</ui5-title>
        <div slot="snappedHeading">...</div>
        <p slot="content">...</p>
        <p slot="snappedContent">...</p>
        <ui5-tag color-scheme="7">...</ui5-tag>
        <ui5-toolbar slot="actionsBar">...</ui5-toolbar>
        <ui5-toolbar slot="navigationBar">
            <ui5-toolbar-button design="Transparent" icon="share"></ui5-toolbar-button>
            <ui5-toolbar-button design="Transparent" icon="action-settings"></ui5-toolbar-button>
        </ui5-toolbar>
    </ui5-dynamic-page-title>
    <ui5-list>
        ...
    </ui5-list>
    <ui5-bar slot="footerArea" design="FloatingFooter">
        ...
    </ui5-bar>
</ui5-dynamic-page>

image

@dobrinyonkov
Copy link
Contributor Author

Thanks for pointing those out @nnaydenov, they are fixed now.

@ilhan007 ilhan007 merged commit 3752ce7 into main May 29, 2024
10 checks passed
@ilhan007 ilhan007 deleted the dynamic-page branch May 29, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants