fundamental-ngx icon indicating copy to clipboard operation
fundamental-ngx copied to clipboard

fix(core): default height missing for dynamic page content component

Open dreamweiver opened this issue 1 year ago • 6 comments

Related Issue(s)

closes #https://github.com/SAP/fundamental-ngx/issues/11877

Description

Dynamic Page Content component doesn't have a default height associated with it and hence the child elements which depend on relative sizing like "fdp-table" component is not able to compute the dynamic height for the sticky headers on the fdp-table

here is an example of the stackblitz with the fix(explicitly setting height 100% on <fd-dynamic-page__content>`

Screenshots

Before the fix,

https://github.com/user-attachments/assets/f8bb3896-a9ef-419c-8c55-ed98632019bb

After setting the default height on the fd-dynamic-page__content component,

https://github.com/user-attachments/assets/472cea2c-f6bd-459a-8f2f-bee1bc80599c

Before:

After:

Please check whether the PR fulfills the following requirements

During Implementation
  1. Visual Testing:
  • [x] visual misalignments/updates
  • [x] check Light/Dark/HCB/HCW themes
  • [x] RTL/LTR - proper rendering and labeling
  • [x] responsiveness(resize)
  • [x] Content Density (Cozy/Compact/(Condensed))
  • [x] States - hover/disabled/focused/active/on click/selected/selected hover/press state
  • [x] Interaction/Animation - open/close, expand/collapse, add/remove, check/uncheck
  • [x] Mouse vs. Keyboard support
  • [x] Text Truncation
  1. API and functional correctness
  • [x] check for console logs (warnings, errors)
  • [x] API boundary values
  • [x] different combinations of components - free style
  • [x] change the API values during testing
  1. Documentation and Example validations
  • [x] missing API documentation or it is not understandable
  • [x] poor examples
  • [x] Stackblitz works for all examples
  1. Accessibility testing
  2. Browser Testing - Edge, Safari, Chrome, Firefox
PR Quality
  • [x] the commit message(s) follows the guideline: https://github.com/SAP/fundamental-ngx/blob/main/CONTRIBUTING.md
  • [x] tests for the changes that have been done
  • [x] all items on the PR Review Checklist are addressed : https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist
  • [x] Run npm run build-pack-library and test in external application
  • [x] update README.md
  • [x] Breaking Changes Wiki

dreamweiver avatar Sep 30 '24 10:09 dreamweiver

Deploy Preview for fundamental-ngx ready!

Name Link
Latest commit f473db2216d2fcb258c127f681bc13a6ce5f3e47
Latest deploy log https://app.netlify.com/sites/fundamental-ngx/deploys/673321a186702d0008808683
Deploy Preview https://deploy-preview-12489--fundamental-ngx.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Sep 30 '24 10:09 netlify[bot]

Visit the preview URL for this PR (updated for commit f473db2):

https://fundamental-ngx-gh--pr12489-fix-core-default-he-f1crsh2f.web.app

(expires Fri, 15 Nov 2024 15:03:39 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff

github-actions[bot] avatar Sep 30 '24 10:09 github-actions[bot]

I understand the issue is that we need to get the header fixed, but we don't want to be seeing the table scrolling up above the header. I'm not sure if there is a css only fix for this problem, may require some js. But this PR fixes one problem and introduces another

Screenshot 2024-10-01 at 3 10 24 PM

i think same, if header should be sticky then it should be aligned in absolute top of the container.

dpavlenishvili avatar Oct 01 '24 19:10 dpavlenishvili

I understand the issue is that we need to get the header fixed, but we don't want to be seeing the table scrolling up above the header. I'm not sure if there is a css only fix for this problem, may require some js. But this PR fixes one problem and introduces another

Screenshot 2024-10-01 at 3 10 24 PM

That is because the fd-dynamic-page-content component which contains the fdp-table as child element has a padding at the top (16px) and this is a free space for the fdp-table to occupy and hence the scrolling content of the fdp-table is visible.

Screenshot 2024-10-06 at 14 14 49

so I don't think it is a problem with the fdp-table component's sticky header implementation, but rather an issue with fd-dynamic-page-content and how it renders its child element. if we remove the top padding, then you won't see any table content upon scrolling

dreamweiver avatar Oct 06 '24 09:10 dreamweiver

@dpavlenishvili @mikerodonnell89 , there is a way to fix this issue via CSS(remove top padding on fd-dynamic-page-content and add margin-top for fdp-table) But i don think its a good idea to do this on the component level, because all these components are suppose to be generic, we shouldn't add logic or code specific to other components behaviour.

I believe this should be handled by the consuming application,by introducing custom CSS styles, something like this https://stackblitz.com/edit/sjxljz-xxe577?file=src%2Fstyles.scss

let me know what you guys think about this.

dreamweiver avatar Oct 06 '24 09:10 dreamweiver

But i don think its a good idea to do this on the component level, because all these components are suppose to be generic, we shouldn't add logic or code specific to other components behaviour.

We do try to practice this, but sometimes need to make exceptions to the rule, and i think this would be an ok example of when to do that. I don't think we want to add a change in to the library that breaks the visuals (see my earlier comment), and then additionally require that the dev needs to add some custom css on their end. So I think for this specific feature, it's best to add a custom class that bends the separation-of-concerns rule a bit.

mikerodonnell89 avatar Oct 14 '24 19:10 mikerodonnell89

But i don think its a good idea to do this on the component level, because all these components are suppose to be generic, we shouldn't add logic or code specific to other components behaviour.

We do try to practice this, but sometimes need to make exceptions to the rule, and i think this would be an ok example of when to do that. I don't think we want to add a change in to the library that breaks the visuals (see my earlier comment), and then additionally require that the dev needs to add some custom css on their end. So I think for this specific feature, it's best to add a custom class that bends the separation-of-concerns rule a bit.

@mikerodonnell89, as agreed I have added the custom CSS styles to support an FDp-table with a sticky header inside a dynamic page content component.

could you review it when you are free :)

dreamweiver avatar Oct 23 '24 12:10 dreamweiver

@droshev could you merge the PR, because i don't have the necessary permission

dreamweiver avatar Nov 12 '24 09:11 dreamweiver