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.

Sign up for GitHub

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

Jump to bottom

feat(ui5-dynamic-page): proper docs added #8130

Merged
merged 1 commit into from
Feb 5, 2024
Merged

feat(ui5-dynamic-page): proper docs added #8130

merged 1 commit into from
Feb 5, 2024

Conversation

PetyaMarkovaBogdanova
Copy link
Contributor

Dynamic Page docs added.
Dynamic behaviour added to the buttons inside Dynamic page basic story.

@nnaydenow
Copy link
Contributor

Rebase to main and align with latest jsdoc standard. You can use documentation in #8123.

@PetyaMarkovaBogdanova
Copy link
Contributor Author

Rebase to main and align with latest jsdoc standard. You can use documentation in #8123.

Thanks, I was just intending to do the same

@dobrinyonkov
Copy link
Contributor

This looks good. Let's aslo add the Overview, Responsive Behavior, Usage, Keyboard Handling sections. We can look into other coponents and dynamic page in ui5 classic as analogy.

dobrinyonkov
dobrinyonkov reviewed Jan 29, 2024
View reviewed changes
packages/fiori/src/DynamicPageHeaderActions.ts Outdated
* @extends sap.ui.webc.base.UI5Element
* @tagname ui5-dynamic-page-header-actions
* @public
Copy link
Contributor

Choose a reason for hiding this comment

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

This may actually stay private. Is only used internally as for now.

...ound/_stories/fiori/DynamicPage/DynamicPageHeaderActions/DynamicPageHeaderActions.stories.ts Outdated
${storyFn()}
`;

export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may remove this when we make it private.

packages/fiori/src/DynamicPage.ts Outdated
@@ -34,14 +34,68 @@ const SCROLL_DEBOUNCE_RATE = 0; // ms
/**
* @class
*
* <h3 class="comment-api-title">Overview</h3>
* A layout control, representing a web page, consisting of a title, header with dynamic behavior, a content area, and an optional floating footer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be part of the Overview section?

packages/fiori/src/DynamicPage.ts Outdated
* If you don't need the Expanding/Snapping functionality it is better to use the
* <code>ui5-page</code> as a lighter control.
*
* The app can add to the <code>content</code> slot of the ui5-dynamic-page either content that is designed to fit its container (e.g. has 100% height),
Copy link
Contributor

Choose a reason for hiding this comment

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

to the default slot

packages/fiori/src/DynamicPageTitle.ts Outdated
@@ -25,13 +25,34 @@ import {
/**
* @class
*
* <h3 class="comment-api-title">Overview</h3>
* Title of the <code>DynamicPage</code> .
Copy link
Contributor

Choose a reason for hiding this comment

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

Also in the Overview section

packages/fiori/src/DynamicPageTitle.ts Outdated
* information regarding the object that will always remain visible while scrolling.
*
* <b>Note:</b> The <code>actions</code> slot accepts any UI5 web component, but it`s
* recommended to use components, suitable for <code>ui5-toolbar</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • recommended to use ui5-toolbar.

packages/playground/_stories/fiori/DynamicPage/DynamicPageTitle/DynamicPageTitle.stories.ts
argTypes,
} as Meta<DynamicPageTitle>;

const Template: UI5StoryArgs<DynamicPageTitle, StoryArgsSlots> = (args) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may use the args properties here to make the template dynamic and the controls can then will change the output when modified. Same for the slots.

packages/playground/_stories/fiori/DynamicPage/DynamicPageHeader/DynamicPageHeader.stories.ts
argTypes,
} as Meta<DynamicPageHeader>;

const Template: UI5StoryArgs<DynamicPageHeader, StoryArgsSlots> = (args) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may use the args properties/slots here to make the template dynamic and the controls can then will change the output when modified.

packages/playground/_stories/fiori/DynamicPage/DynamicPage.stories.ts Outdated
return page.shadowRoot.querySelector(".ui5-dynamic-page-scroll-container");
}

document.getElementById("toggleFooterBtn").addEventListener("click", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This script tag throws an error in the console and prevents modifying the example using the storybook controls. If I remember correctly we talk that they are not needed, as they were only added for testing purpose.

packages/playground/_stories/fiori/DynamicPage/DynamicPage.stories.ts
@@ -48,7 +48,7 @@ const Template: UI5StoryArgs<DynamicPage, StoryArgsSlots> = (args) => {
>
Copy link
Contributor

Choose a reason for hiding this comment

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

    ?header-snapped="${ifDefined(args.headerSnapped)}"
    ?header-pinned="${ifDefined(args.headerPinned)}"
    ?show-footer="${ifDefined(args.showFooter)}"
    >

This needs to be adjusted as above in order to correctly propagate the values

@PetyaMarkovaBogdanova PetyaMarkovaBogdanova force-pushed the dynamic-page-petya-docs branch 2 times, most recently from 7dd2186 to 69b124d Compare February 2, 2024 12:59
@dobrinyonkov dobrinyonkov merged commit b47f38b into dynamic-page Feb 5, 2024
8 checks passed
@dobrinyonkov dobrinyonkov deleted the dynamic-page-petya-docs branch February 5, 2024 09:22
dobrinyonkov added a commit to dobrinyonkov/ui5-webcomponents that referenced this pull request Feb 5, 2024
@dobrinyonkov @kineticjs @PetyaMarkovaBogdanova
Dynamic page (#4)
6855665 
* test: do not merge

* feat(ui5-dynamic-page): poc

* feat(ui5-dynamic-page): snap on scroll pin expand collapse

* feat(ui5-dynamic-page): add floating footer styling and animation (SAP#7857)

* feat(ui5-dynamic-page): snap on scroll pin expand collapse

* feat(ui5-dynamic-page): add breadcrumbs responsiveness (SAP#7844)

* feat(ui5-dynamic-page): ensure visibility of content outside overflow (SAP#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): footer no longer takes space when hidden (SAP#7904)

* 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): add header padding (SAP#7921)

* feat(ui5-dynamic-page): add responsive paddings (SAP#7923)

* build: align with new build process

* feat(ui5-dynamic-page): fit content that has 100% height (SAP#7928)

* 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 (SAP#7990)

* 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): theme styling (SAP#7966)

fix(ui5-dynamic-page): apply theme styles

Co-authored-by: PetyaMarkovaBogdanova <[email protected]>

* feat(ui5-dynamic-page): screen reader support (SAP#7997)

* feat(ui5-dynamic-page): screen reader support

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

* feat(ui5-dynamic-page): add general interaction tests (SAP#8056)

* docs(ui5-dynamic-page): story and docs added (SAP#8031)

fix(ui5-dynamic-page): story and docs added

Co-authored-by: PetyaMarkovaBogdanova <[email protected]>

* feat(ui5-dynamic-page): add general API tests (SAP#8115)

* feat(ui5-dynamic-page): proper docs added (SAP#8130)

Co-authored-by: PetyaMarkovaBogdanova <[email protected]>

* chore: align docs to new build scripts

* chore: use framework's debounce method

---------

Co-authored-by: PetyaMarkovaBogdanova <[email protected]>
Co-authored-by: Diana Pazheva <[email protected]>
Co-authored-by: PetyaMarkovaBogdanova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dobrinyonkov dobrinyonkov dobrinyonkov approved these changes

@kineticjs kineticjs Awaiting requested review from kineticjs

@simonarangelova simonarangelova Awaiting requested review from simonarangelova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PetyaMarkovaBogdanova @nnaydenow @dobrinyonkov