-
Notifications
You must be signed in to change notification settings - Fork 381
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
ListView - Added ability to apply header styling #1700
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the first contribution to this project.
i have no idea how the other 2 commits got in here. Oops? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bradleypregon, many thanks and sorry for the long delay.
The approach seems fine to me, but I added 2 comments regarding reference to CSS classes by name as that approach often causes issues when the underlying components are updated and classes change. Please let me know if you have any questions
root: { | ||
lineHeight: "normal", | ||
minHeight: '17px', | ||
'.ms-DetailsHeader-cell': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referencing the class name is likely to fail in the future as we have no control over style updates from external components. Can you think of a more reliable approach?
lineHeight: 'normal', | ||
height: '100%', | ||
}, | ||
'.ms-DetailsHeader-cellTitle': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above regarding class name
Ah, I understand your reasoning about the class names. I'm not sure of a good alternative at the moment, I'll have to do some digging and tinkering. |
Hi @bradleypregon, Are you still working on this PR? Do you need help? |
Hello @michaelmaillot, Apologies for the late reply. Yes, I am still working on the PR, although I have not looked at it for some time, and I'm still stuck. I tried a few different approaches, one involving the use of |
What's in this Pull Request?
(This is my 1st open source contribution and I'm still fresh at TS/JS... bear with me.)
I added the ability to apply custom styles to the header of a ListView via optional prop headerClassName.
Something I was not able to implement was the ability to modify the font styling of the header text (size, weight, etc), so I set the header min-height to 17px which seemed small enough to scrunch everything without clipping any text - I tried testing as many scenarios as I could. Theoretically, to modify the font styling, I think you could add another styling prop to ListView, and apply that prop to '.ms-DetailsHeader-cellTitle' in ListView.tsx... I can experiment with this at another time.
For my use case, although likely unconventional, I'm using a ListView in a tight spot on a page and need all the real estate being taken up by the default padding (padding-top of 16px) and line height (42px iirc).
Let me know if I'm, at all, doing this incorrectly (lol).