fix: (Platform) remove dependency on position: fixed for Dynamic Page #4343

kavya-b · 2021-01-15T06:13:14Z

Please provide a link to the associated issue.

#4337

Please provide a brief summary of this pull request.

This PR fixes issue with having position: fixed in Dynamic Page that was causing other side-effects like appearing over the entire page and not taking up its parent's space. We are now using position: static. Also added an example for using with Flexible Column Layout.

Before:

After:

This PR also fixes issue where dynamic page tab styles were getting applied to other fd-tabs used within the page. It fixes the issue by applying more specificity to the dynamic page tab styles application.

Before:

After:

Within tabbed dynamic page example, styles will only apply to dynamic page tabs, not user-provided tabs in content:

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/main/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist

Documentation checklist:

[n/a] update README.md
[n/a] Breaking Changes Wiki
Documentation Examples
Stackblitz works for all examples

netlify · 2021-01-15T06:19:51Z

Deploy preview for fundamental-ngx ready!

Built with commit a02b604

https://deploy-preview-4343--fundamental-ngx.netlify.app

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts

InnaAtanasova

Hi @kavya-b , unfortunately the issue is not fixed. The issue was detected by an application using Flexible Column Layout together with Platform Dynamic Page.
The way you can reproduce the issue is:

Go to flexible-column-layout-example.component.html and as a content for #midColumn template add the Dynamic Page example with Tabs
Add the necessary imports for Button, Breadcumb, Bar, Toolbar, etc
When you test the example now you will have this:

Open the example:

Click the button to show the second column of the Flexible column layout:

The result is:

Because the position of the header is fixed, and you specify left:0, etc the header is now taking the whole width of the page, but not the width of the container. In this case the header should fit in the second column space.
Let me know if you need more info

kavya-b · 2021-01-15T17:53:51Z

Hi @kavya-b , unfortunately the issue is not fixed. The issue was detected by an application using Flexible Column Layout together with Platform Dynamic Page.
The way you can reproduce the issue is:

Go to flexible-column-layout-example.component.html and as a content for #midColumn template add the Dynamic Page example with Tabs

Add the necessary imports for Button, Breadcumb, Bar, Toolbar, etc

When you test the example now you will have this:

Open the example:

Click the button to show the second column of the Flexible column layout:

The result is:

Because the position of the header is fixed, and you specify left:0, etc the header is now taking the whole width of the page, but not the width of the container. In this case the header should fit in the second column space.
Let me know if you need more info

@InnaAtanasova I see.. seems to be a problem with position: fixed being used at multiple places to achieve the sticking header on scroll. This issue with flexible column layout might need more time and effort to work on, and will probably not be a small fix that can be part of this PR. Should I raise a separate issue for flex column issue and consider this PR as only addressing the tab problem within dynamic page context; or should I close this PR and address both issues at a later point? What do you suggest?

JKMarkowski · 2021-01-18T12:25:16Z

@InnaAtanasova I see.. seems to be a problem with position: fixed being used at multiple places to achieve the sticking header on scroll. This issue with flexible column layout might need more time and effort to work on, and will probably not be a small fix that can be part of this PR. Should I raise a separate issue for flex column issue and consider this PR as only addressing the tab problem within dynamic page context; or should I close this PR and address both issues at a later point? What do you suggest?

Hi @kavya-b This issue with fixed actually caused some other errors.
Can't header become absolute, or even relative, without all of these css properties (top/right/left/position) set by JS?
Content can be scrollable and header can stay at same position without even using fixed.

This can bring some unexpected behaviour for users - since we're pretty ready for release, I suggest
Adding necessary changes to this PR that will be applied only after setting some option (ex. @Input() fixed enabled by default). Or merge this PR, which partially fixes bug with tabs and address fixed problem in another PR/issue

@InnaAtanasova what do you think?

InnaAtanasova · 2021-01-18T14:04:12Z

@InnaAtanasova I see.. seems to be a problem with position: fixed being used at multiple places to achieve the sticking header on scroll. This issue with flexible column layout might need more time and effort to work on, and will probably not be a small fix that can be part of this PR. Should I raise a separate issue for flex column issue and consider this PR as only addressing the tab problem within dynamic page context; or should I close this PR and address both issues at a later point? What do you suggest?

Hi @kavya-b This issue with fixed actually caused some other errors.
Can't header become absolute, or even relative, without all of these css properties (top/right/left/position) set by JS?
Content can be scrollable and header can stay at same position without even using fixed.

This can bring some unexpected behaviour for users - since we're pretty ready for release, I suggest
Adding necessary changes to this PR that will be applied only after setting some option (ex. @Input() fixed enabled by default). Or merge this PR, which partially fixes bug with tabs and address fixed problem in another PR/issue

@InnaAtanasova what do you think?

I am not in favour of merging this PR as it's not really fixing the issue. If we merge it this means that the component can be used in only one specific case where it's used as a page layout and takes the whole width of the window. From what I see most of the use cases are when the component is part of a wrapper, not limited to Flexible column layout. The position should not be fixed. @JKMarkowski solution could work and is a better approach that the current using the styles.

kavya-b · 2021-01-19T05:26:35Z

@InnaAtanasova @JKMarkowski
After adding a fixed input that is false by default and conditionally applying the styles, I was able to achieve this, but it has a few issues:

2021-01-19_10-04-21.mp4

scrolling expands/collapses the header but the header does not stick anymore since position: fixed is no longer applied. @JKMarkowski could you give some pointers in the direction of resolving this issue? This may not be relevant, but when first developing the component, I used cdkScrollables on the dynamic page content but it was too complicated in usage, and had some side-effects like double scrollbars(one on content, one on page scroll), and if multiple cdkScrollables were used in the page, our scrolling logic would be applied to all cdkScrollables. Since the documentation on cdkScrollable is not very comprehensive and we had to tweak things without knowing what other side-effects might appear later on, we decided(in PR 3515, #1 #2 #3 #4 )to listen to only page scrolls for which I had to use position: fixed (position: sticky would have been simpler but has no IE support) and the inline styles.
floating footer bar still comes over the entire page: this is because position: absolute is applied in the bar styles. However, if changed to relative it is contained within 2nd column, but is not floating any more and is only visible if you scroll to the end of the page.
also, it seems flexible column layout also only provides one scroll at the page level, whereas it should probably be one scroll for each column.

kavya-b · 2021-01-19T11:41:47Z

Update: I am trying something using position: absolute which(if everything works right) might remove the need for having fixed in styles or as an input attribute.

Achieving it for tabs with scrolling is a bit more challenging; I will continue working on it.

kavya-b · 2021-01-20T12:57:27Z

@InnaAtanasova @JKMarkowski have removed position: fixed and cdkScrollable in library now, achieving the same with position: absolute and scroll listeners. Also added an example for usage with Flexible Column Layout.
Please take a look.

JKMarkowski

Hi @kavya-b, I added some comments. IMO current dynamic page is too complicated.
Dynamic page looks like that:

Most of the things are not needed just to have it in this way.
The only thing you will need to calculate for this proposition is height of content.

So you can apply on init, or header expand just a height of content:

100vh - distance from top - offset(which is added by user)

Let me know what do you think about this approach, if it does make any sense.

In general right now it works much better than before. Great job!

JKMarkowski · 2021-01-21T10:20:09Z

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.scss

+.header-sticker, .tab-sticker {
+    position: absolute;
+    width: 100%;
    top: 0;
 }


Could you keep it static instead of absolute?

changed to static as suggested

JKMarkowski · 2021-01-21T10:25:42Z

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts

+            const contentComponentElement = this.contentComponent.getElementRef().nativeElement;
+
+            contentComponentElement.style.top = this.header.nativeElement.offsetHeight + 'px';
+            this._addClassNameToCustomElement(contentComponentElement, 'content-sticker');


There is no need to add top anywhere. You can easily do it without applying top by applying static/relative positions

removed usage of top. Thanks for showing me this approach, Jedrzej!

JKMarkowski · 2021-01-21T10:26:01Z

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts

+        this._addClassNameToCustomElement(tabList, 'tab-sticker');
+        tabList.style.top = this.header.nativeElement.offsetHeight + 'px';


Same, with static/relative you won't need to add it

removed usage of top

JKMarkowski · 2021-01-21T10:29:26Z

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts

+        if (tabContent) {
+            tabContent.forEach((contentItem) => {
+                const element: HTMLElement = contentItem
+                    .getElementRef()
+                    .nativeElement.querySelector('.fd-dynamic-page__content');
+                if (element) {
+                    element.style.top = this.header.nativeElement.offsetHeight + tabList.offsetHeight + 'px';
+                }
            });
        }


That's also the thing that you won't need with relative/static

removed usage of top

JKMarkowski · 2021-01-21T10:30:06Z

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts

+        this._addClassNameToCustomElement(tabList, CLASS_NAME.dynamicPageTabs);
+        this._addClassNameToCustomElement(tabList, CLASS_NAME.dynamicPageTabsAddShadow);
+        this._renderer.setStyle(tabList, 'z-index', 1);
+
+        if (this.header) {
+            this._renderer.setStyle(this.header.nativeElement, 'z-index', 2);
+        }
+
+        if (this.size) {
+            this._setTabsSize(this.size, tabList);
+        }
+
+        if (!this.headerComponent?.collapsible) {
+            return;
+        }
+
+        const pinCollapseShadowElement = this._elementRef.nativeElement.querySelector(
+            '.fd-dynamic-page__collapsible-header-visibility-container'
+        );
+
+        if (pinCollapseShadowElement) {
+            this._addClassNameToCustomElement(
+                pinCollapseShadowElement,
+                CLASS_NAME.dynamicPageCollapsibleHeaderPinCollapseNoShadow
+            );
        }


Let's recheck if this is still needed, when applied static/relative position

this is still needed as it is unrelated to positioning. It adds some necessary modifier classes to the tabs.

kavya-b · 2021-01-21T11:41:32Z

Hi @kavya-b, I added some comments. IMO current dynamic page is too complicated.
Dynamic page looks like that:

Most of the things are not needed just to have it in this way.
The only thing you will need to calculate for this proposition is height of content.

So you can apply on init, or header expand just a height of content:
100vh - distance from top - offset(which is added by user)
Let me know what do you think about this approach, if it does make any sense.

In general right now it works much better than before. Great job!

I see.. did not know about this approach of using viewheight for calculating content height. So we are keeping header and footer as is(with static/relative) and content position will be static/relative but with a height of the calculated value; is my understanding correct? What would the offset do? Would it be provided as an @Input by the user?

I still somehow feel that setting top might come into picture when using tabbed content though 😅 Anyway, I will try this approach.

JKMarkowski · 2021-01-21T13:02:13Z

I see.. did not know about this approach of using viewheight for calculating content height. So we are keeping header and footer as is(with static/relative) and content position will be static/relative but with a height of the calculated value; is my understanding correct? What would the offset do? Would it be provided as an @Input by the user?

I still somehow feel that setting top might come into picture when using tabbed content though Anyway, I will try this approach.

I was thinking about offset just in case user wants to put something below the dynamic page. It can be provided by the user with @Input(). Just to make it more flexible.
For tabbed content this should be fine.

The only issue can be meet with stacked content, but user should handle it by himself in this case

…nt tabs

- also add z-index so that tabs in dynamic page have more weight than tabs in content, avoiding one visible behind the other on scrolling

- minor documentation selector name refactoring

fix failing unit test

-removed unnecessary code

JKMarkowski

Looks much better. Great Job!

InnaAtanasova

Awesome work!

kavya-b added bug Something isn't working platform platform labels Jan 15, 2021

kavya-b requested review from JKMarkowski and a team January 15, 2021 06:13

kavya-b self-assigned this Jan 15, 2021

DeepakSap14 approved these changes Jan 15, 2021

View reviewed changes

Betrozov reviewed Jan 15, 2021

View reviewed changes

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts Outdated Show resolved Hide resolved

libs/platform/src/lib/components/dynamic-page/dynamic-page.component.ts Outdated Show resolved Hide resolved

kavya-b force-pushed the fix-platform-dynamic-page-tabs branch from f192da0 to 6b030e1 Compare January 15, 2021 11:06

Betrozov approved these changes Jan 15, 2021

View reviewed changes

InnaAtanasova self-requested a review January 15, 2021 14:33

InnaAtanasova requested changes Jan 15, 2021

View reviewed changes

kavya-b added this to the Sprint 55 - ariba milestone Jan 19, 2021

kavya-b force-pushed the fix-platform-dynamic-page-tabs branch from 6b030e1 to ad29bd1 Compare January 20, 2021 12:26

kavya-b requested a review from InnaAtanasova January 20, 2021 12:55

kavya-b changed the title ~~fix: (Platform) add styles only to Dynamic Page tabs and not to content tabs~~ fix: (Platform) remove dependency on position: fixed for Dynamic Page Jan 21, 2021

kavya-b force-pushed the fix-platform-dynamic-page-tabs branch from ad29bd1 to b95fcda Compare January 21, 2021 09:01

JKMarkowski suggested changes Jan 21, 2021

View reviewed changes

kavya-b added 4 commits January 22, 2021 20:36

fix: (Platform) add styles only to Dynamic Page tabs and not to conte…

0a52ee1

…nt tabs

refactor nested ifs

4ad9ed6

- also add z-index so that tabs in dynamic page have more weight than tabs in content, avoiding one visible behind the other on scrolling

remove dependency on position:fixed

8855a2a

- minor documentation selector name refactoring

remove unused directive

153b185

fix failing unit test

changed to position:static and added input offset

a02b604

-removed unnecessary code

kavya-b force-pushed the fix-platform-dynamic-page-tabs branch from b95fcda to a02b604 Compare January 22, 2021 15:06

kavya-b requested a review from JKMarkowski January 22, 2021 15:08

JKMarkowski approved these changes Jan 25, 2021

View reviewed changes

InnaAtanasova approved these changes Jan 25, 2021

View reviewed changes

kavya-b merged commit f80c9ad into main Jan 27, 2021

kavya-b deleted the fix-platform-dynamic-page-tabs branch January 27, 2021 04:38

kavya-b linked an issue Jan 27, 2021 that may be closed by this pull request

Tabs used inside DynamicPageContent #4337

Closed

kavya-b mentioned this pull request Jan 27, 2021

Stackblitz examples not working in PLATFORM #4392

Closed

28 tasks

manjunathanagaraj mentioned this pull request Jan 29, 2021

Sprint 55 - ariba: Summary #4524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (Platform) remove dependency on position: fixed for Dynamic Page #4343

fix: (Platform) remove dependency on position: fixed for Dynamic Page #4343

kavya-b commented Jan 15, 2021 •

edited

netlify bot commented Jan 15, 2021 •

edited

InnaAtanasova left a comment

kavya-b commented Jan 15, 2021

JKMarkowski commented Jan 18, 2021 •

edited

InnaAtanasova commented Jan 18, 2021

kavya-b commented Jan 19, 2021

kavya-b commented Jan 19, 2021

kavya-b commented Jan 20, 2021

JKMarkowski left a comment •

edited

JKMarkowski Jan 21, 2021

kavya-b Jan 22, 2021

JKMarkowski Jan 21, 2021

kavya-b Jan 22, 2021

JKMarkowski Jan 21, 2021

kavya-b Jan 22, 2021

JKMarkowski Jan 21, 2021 •

edited

kavya-b Jan 22, 2021

JKMarkowski Jan 21, 2021

kavya-b Jan 22, 2021

kavya-b commented Jan 21, 2021

JKMarkowski commented Jan 21, 2021

JKMarkowski left a comment

InnaAtanasova left a comment

		this._addClassNameToCustomElement(tabList, 'tab-sticker');
		tabList.style.top = this.header.nativeElement.offsetHeight + 'px';

fix: (Platform) remove dependency on position: fixed for Dynamic Page #4343

fix: (Platform) remove dependency on position: fixed for Dynamic Page #4343

Conversation

kavya-b commented Jan 15, 2021 • edited

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Please check whether the PR fulfills the following requirements

netlify bot commented Jan 15, 2021 • edited

InnaAtanasova left a comment

Choose a reason for hiding this comment

kavya-b commented Jan 15, 2021

JKMarkowski commented Jan 18, 2021 • edited

InnaAtanasova commented Jan 18, 2021

kavya-b commented Jan 19, 2021

kavya-b commented Jan 19, 2021

kavya-b commented Jan 20, 2021

JKMarkowski left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JKMarkowski Jan 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavya-b commented Jan 21, 2021

JKMarkowski commented Jan 21, 2021

JKMarkowski left a comment

Choose a reason for hiding this comment

InnaAtanasova left a comment

Choose a reason for hiding this comment

kavya-b commented Jan 15, 2021 •

edited

netlify bot commented Jan 15, 2021 •

edited

JKMarkowski commented Jan 18, 2021 •

edited

JKMarkowski left a comment •

edited

JKMarkowski Jan 21, 2021 •

edited