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

fix(shell-panel): Fix visual issues in displayMode="float-all" #11410

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

macandcheese
Copy link
Contributor

@macandcheese macandcheese commented Jan 30, 2025

Related Issue: #9926

Summary

Adds some styling to address the visual issues in Shell Panel.

Screenshot 2025-01-29 at 6 58 09 PM
Screenshot 2025-01-29 at 6 57 27 PM

Note - I went down the rabbit hole of trying to conditionally pass corner-radius to slotted Actions, Panels, etc. - it would involve a lot of internal tokens and conditional states to handle various arrangements of components. This could be something to look at in the future but I'm not sure we want to handle this here.

Screenshot 2025-01-29 at 6 54 39 PM

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 30, 2025
@macandcheese macandcheese added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 30, 2025
@macandcheese macandcheese marked this pull request as ready for review January 30, 2025 02:56
@macandcheese macandcheese added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 30, 2025
@macandcheese macandcheese added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 30, 2025
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

LGTM! Do we need tokens for customizing these new styles?

.float-all {
max-block-size: var(--calcite-internal-shell-panel-max-height, calc(100% - 1rem));
box-shadow: var(--calcite-shadow-sm);
border-radius: var(--calcite-corner-radius-round);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a token for customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this in - it exposes what may be a common thing across our components - unintentional clipping of children that don't have similar corner-radius adjustments. A more severe example of what I mentioned in the title post:

Screenshot 2025-01-30 at 11 21 53 AM

Lot of ways to solve this...

  • Pass responsibility to user to set corner-radius overrides as needed on slotted Panel, Action Bar, Action, etc. (I think they can do this with current functionality, could double check the internally-rendered expand button).

  • Look to solve this somehow across components through some kind of clipping / overflow settings. I'm not sure if it's possible (or desirable) - and seems like it'd be challenging to work with how many "levels" of nesting we have where children need to validly escape.

  • Add a handful of internal custom css properties for action-group and action-bar. We’d then need to conditionally set the corner radius of those components, and slotted panels - for what would probably be a dozen or more configurations of: Shell Panel position, layout, if the Actions are slotted directly in Action Bar vs. in groups, etc.

I kind of lean towards the first one, for simplicities sake - but curious if this has cropped up anywhere else yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this happen in Popover and also in Dialog (see the top corners)
CleanShot 2025-01-30 at 11 45 38@2x

Spoke to @jcfranco about this a while ago and he had similar concerns you outline in your second bullet, but also thought it could be worth looking into. Might adversely affect floating ui slotted into a Dialog...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we could approach it in a few ways, all have pros / cons. For now, I think I'll leave this as-is, since it's not unique to Shell Panel, and should be done in a way that addresses all patterns like this.

Copy link
Contributor Author

@macandcheese macandcheese Feb 1, 2025

Choose a reason for hiding this comment

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

@jcfranco ok to proceed for now (with option 1 - not handling this from Shell Panel for now) ?

@ashetland
Copy link
Contributor

Screenshots are looking good, but the sizing behavior is weird. The edge floats in space.
CleanShot 2025-01-30 at 09 51 37@2x

@ashetland
Copy link
Contributor

I see we removed that requirement from the issue, so maybe this can be ignored in this pr?

@macandcheese
Copy link
Contributor Author

I see we removed that requirement from the issue, so maybe this can be ignored in this pr?

Yeah, the original issue asked to remove, but that would be breaking - I think we should leave as-is, since this PR doesn't change that behavior. This issue should be additive improvement: #5652

Copy link
Contributor

@ashetland ashetland left a comment

Choose a reason for hiding this comment

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

Visuals lookin' good!

@macandcheese macandcheese added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants