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.

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

bradleypregon
Copy link

Q A
Bug fix? [ ]
New feature? [x]
New sample? [ ]
Related issues? Partially adds #410

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).

Copy link

@github-actions github-actions bot left a 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.

@bradleypregon
Copy link
Author

i have no idea how the other 2 commits got in here. Oops?

Copy link
Collaborator

@joelfmrodrigues joelfmrodrigues left a 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': {
Copy link
Collaborator

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': {
Copy link
Collaborator

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

@bradleypregon
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants