Skip to content

[Dialog] - content background should be white #10809

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.

Sign up for GitHub

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

Jump to bottom
Closed
2 of 6 tasks
ashetland opened this issue Nov 20, 2024 · 16 comments
Closed
2 of 6 tasks

[Dialog] - content background should be white #10809

ashetland opened this issue Nov 20, 2024 · 16 comments
Assignees
@geospatialem @SkyeSeitz @DitwanP @ashetland @brittneytewks
Labels
4 - verified Issues that have been tested, confirmed as mitigated, and are ready to close. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Calcite (design) Issues logged by Calcite designers. calcite-components Issues specific to the @esri/calcite-components package. estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. figma changes Issues that require additions or updates to the Figma UI Kit where no `design` label exists impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone p - high Issue should be addressed in the current milestone, impacts component or core functionality spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. visual changes Issues with visual changes that are added for consistency, but are not backwards compatible.
Milestone
2024-12-17 - Dec ...

Comments

@ashetland
Copy link
Contributor

Check existing issues

Actual Behavior

The background of the content area is color.background.

Expected Behavior

The background is color.foreground.1.

Reproduction Sample

https://codepen.io/ashetland/pen/WNVqpjw?editors=100

Reproduction Steps

Compare the reproduction sample of Dialog with Modal.

Reproduction Version

v2.13

Relevant Info

cc @jcfranco @geospatialem

Regression?

No response

Priority impact

impact - p1 - need for current milestone

Impact

Modal was deprecated in favor of Dialog, but their background colors have different defaults. Correcting this to match Modal will aid migration and will address active feedback from teams.

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/calcite-ui-icons
  • @esri/eslint-plugin-calcite-components

Esri team

Calcite (design)
@ashetland ashetland added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Nov 20, 2024
@github-actions github-actions bot added Calcite (design) Issues logged by Calcite designers. calcite-components Issues specific to the @esri/calcite-components package. impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone labels Nov 20, 2024
@ashetland ashetland changed the title [Dialog] - content background should white [Dialog] - content background should be white Nov 20, 2024
@geospatialem geospatialem added estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. visual changes Issues with visual changes that are added for consistency, but are not backwards compatible. p - high Issue should be addressed in the current milestone, impacts component or core functionality and removed needs triage Planning workflow - pending design/dev review. labels Nov 20, 2024
@driskull
Copy link
Member

driskull commented Nov 20, 2024 •
edited
Loading

Modal was deprecated in favor of Dialog, but their background colors have different defaults. Correcting this to match Modal will aid migration and will address active feedback from teams.

Its just using the calcite-panel. Why would the panel use a different background color in this instance?

Modal was deprecated in favor of Dialog, but their background colors have different defaults. Correcting this to match Modal will aid migration and will address active feedback from teams.

@ashetland why not change panel or introduce a property on panel to configure the background appearance if it is different depending on certain use cases.

@macandcheese
Copy link
Contributor

Prior to the change Modal behaved the same way (default to white, option to change via css property). Similarly to how we have a different default padding in Dialog than we do in Panel.

This should ease migration from Modal while also allowing Dialog to remain flexible for the increased set of use cases it serves - where both the default (now white) or other values (grey, primarily, but depending on use case other options) can be set via the existing css property.

@ashetland
Copy link
Contributor Author

ashetland commented Nov 20, 2024 •
edited
Loading

Changing the default background color of Panel would also require some design tweaks to Block and other components. It would be a large change in the design pattern.

@driskull
Copy link
Member

I guess my concern is the inconsistency.

If we have valid use cases for when a panel should have a white background then maybe we should document them by having a prop where users can configure it rather than having to set CSS vars to a specific value.

@macandcheese
Copy link
Contributor

macandcheese commented Nov 20, 2024 •
edited
Loading

This would be a direct response to users asking for this change - honestly I don't have concerns about that level of difference and I don't think we want to limit that level of customization. This is how Modal functioned before, all we are doing is making the new component match that prior default so migrating between components is simpler.

Making users adjust this via a property seems like an anti-pattern vs. using a css property.

Panel as a standalone component currently does not have many use cases where it shouldn't be the default grey (again, because the primary use case there is slotting in "foreground Blocks") on top of that. Dialog has different use cases, some of which may necessitate an override, but ultimately using the less opinionated white bg makes that a simpler story.

That relationship between Panel / Block will definitely be looked at and we have ideas to improve it, but that's more than a year away post 4.0 at this point.

@jcfranco jcfranco self-assigned this Nov 23, 2024
@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Nov 23, 2024
@jcfranco jcfranco mentioned this issue Nov 23, 2024
Merged
@ashetland ashetland added the figma changes Issues that require additions or updates to the Figma UI Kit where no `design` label exists label Nov 25, 2024
jcfranco added a commit that referenced this issue Nov 26, 2024
@jcfranco
fix(dialog): update content background color to match modal (#10856)
0fd6560 
**Related Issue:** #10809 

## Summary

Updates `dialog`’s content background color to be consistent with
`modal`, easing the team’s migration efforts.
@jcfranco jcfranco added 3 - installed Issues that have been merged to the "dev" branch and/or are ready for QA/QC. and removed 2 - in development Issues that are actively being worked on. labels Nov 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem
Copy link
Member

Reopening to ensure the change only occurs on the internal panel cc @driskull

@geospatialem geospatialem reopened this Dec 12, 2024
@geospatialem geospatialem added the spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. label Dec 12, 2024
driskull added a commit that referenced this issue Dec 13, 2024
@driskull
fix(dialog): no longer modifies slotted panels background color. #10809
d5cd474
@driskull driskull mentioned this issue Dec 13, 2024
Merged
driskull added a commit that referenced this issue Dec 16, 2024
@driskull
fix(dialog): no longer modifies slotted panels background color (#11052)
0c46290 
**Related Issue:** #10809

## Summary

- set slotted panel bg color to initial color
- add story test
@driskull driskull added 3 - installed Issues that have been merged to the "dev" branch and/or are ready for QA/QC. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Dec 16, 2024
@github-actions GitHub Actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been tested, confirmed as mitigated, and are ready to close. and removed 3 - installed Issues that have been merged to the "dev" branch and/or are ready for QA/QC. labels Dec 16, 2024
@geospatialem
Copy link
Member

Verified in 3.0.0-next.65, and also including scenarios where slotted panel's content background is unchanged. 🎉

@ellenupp
Copy link

Thanks, though what if a panel is not directly slotted in the dialog but nested? For example we use panel in shell in dialog and the background color is still white/foreground-1.

@driskull
Copy link
Member

hmmm that might not work. We would need to update the slotted selector to target any element.

@driskull
Copy link
Member

Created a new PR to fix your use case.

@ellenupp
Copy link

Looks good in next.67, thanks!

benelan pushed a commit that referenced this issue Feb 8, 2025
@jcfranco @benelan
fix(dialog): update content background color to match modal (#10856)
f63fd4a 
**Related Issue:** #10809 

## Summary

Updates `dialog`’s content background color to be consistent with
`modal`, easing the team’s migration efforts.
benelan pushed a commit that referenced this issue Feb 8, 2025
@driskull @benelan
fix(dialog): no longer modifies slotted panels background color (#11052)
0df6201 
**Related Issue:** #10809

## Summary

- set slotted panel bg color to initial color
- add story test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geospatialem geospatialem

@SkyeSeitz SkyeSeitz

@DitwanP DitwanP

@ashetland ashetland

@brittneytewks brittneytewks

Labels
4 - verified Issues that have been tested, confirmed as mitigated, and are ready to close. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. Calcite (design) Issues logged by Calcite designers. calcite-components Issues specific to the @esri/calcite-components package. estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. figma changes Issues that require additions or updates to the Figma UI Kit where no `design` label exists impact - p1 - need for current milestone User set priority impact status of p1 - need for current milestone p - high Issue should be addressed in the current milestone, impacts component or core functionality spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. visual changes Issues with visual changes that are added for consistency, but are not backwards compatible.
Projects
None yet
Milestone
2024-12-17 - Dec Milestone
Development

No branches or pull requests
9 participants
@jcfranco @driskull @macandcheese @geospatialem @ellenupp @SkyeSeitz @DitwanP @ashetland @brittneytewks