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

Provide more flexibility with headers and padding of Panels #852

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

lyzadanger
Copy link
Contributor

This PR provides a set of additional props for CardHeader, Panel and Modal to give more flexibility in layout styling.

These changes will cause a visual change to existing Modals: the border under the modal's header will now extend all the way to the edges of the modal. Existing Panels are unaffected. Other props introduced here are additive (no changes to existing usage of these components).

The first problem this work attempts to solve is to make Panel/Modal headers span the full width if there is scrollable content. This is to avoid scroll hint shadows looking funky (see #839).

I also ran into a situation recently in which it was necessary to have more control over the way that padding is applied to Panel content.

The broader fix here, so we don't end up in an arms race, is to not force all Modal content to be in Panels (i.e. allow Modals with arbitrary content, not just Panels). I intend to make the next-generation Dialog component more flexible in that respect.

These changes are documented on the pattern library pages for Card Panel, and Modal.

Fixes #839

@lyzadanger
Copy link
Contributor Author

I can't make any sense of the CodeCov fail. Local test coverage is complete. I'll look in the morning with fresh eyes.

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code coverage issue is legit. It is complaining about this line not being covered.

In order to cover that you need to explicitly provide paddingSize="none" in some test and check that there's no CardContent in that case.

@@ -21,6 +34,13 @@ type ComponentProps = {
* this function as an onClick handler
*/
onClose?: () => void;
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be an empty line before this comment?

@lyzadanger
Copy link
Contributor Author

I think the code coverage issue is legit. It is complaining about this line not being covered.

In order to cover that you need to explicitly provide paddingSize="none" in some test and check that there's no CardContent in that case.

Oh, yes, it's legit! I got so busy yesterday that I didn't get a chance to circle back to this.

@acelaya
Copy link
Contributor

acelaya commented Feb 15, 2023

I think the code coverage issue is legit. It is complaining about this line not being covered.
In order to cover that you need to explicitly provide paddingSize="none" in some test and check that there's no CardContent in that case.

Oh, yes, it's legit! I got so busy yesterday that I didn't get a chance to circle back to this.

The weird thing is that locally it does not complain about that. Only if you open the code coverage report you can see that branch is not covered, but in terms of lines, it is reported as covered, which I think it's not correct.

@lyzadanger
Copy link
Contributor Author

The weird thing is that locally it does not complain about that. Only if you open the code coverage report you can see that branch is not covered, but in terms of lines, it is reported as covered, which I think it's not correct.

Yes, that is my general complaint as well. I did check local coverage before pushing the branch.

Add new props to Panel:

* `scrollable`: Panels no longer scroll their content by default
* `fullWidthHeader`
* `paddingSize`, including a `none` option to turn off padding around
  content
* Forward the `paddingSize` prop if provided
* Set `Panel` to `scrollable`
@lyzadanger lyzadanger force-pushed the scrollable-panel-headers branch from 2824746 to 2a8e099 Compare February 15, 2023 14:25
@lyzadanger lyzadanger merged commit 975df44 into main Feb 15, 2023
@lyzadanger lyzadanger deleted the scrollable-panel-headers branch February 15, 2023 14:27
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.

Address visual balance of border above scrolling content in Panel
2 participants