fundamental-ngx
fundamental-ngx copied to clipboard
fix(core): default height missing for dynamic page content component
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
- 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
- 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
- Documentation and Example validations
- [x] missing API documentation or it is not understandable
- [x] poor examples
- [x] Stackblitz works for all examples
- Accessibility testing
- 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
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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
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
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
i think same, if header should be sticky then it should be aligned in absolute top of the container.
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
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.
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
@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.
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.
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 :)
@droshev could you merge the PR, because i don't have the necessary permission

