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.

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

[popups] Add data-closed style hook #882

Closed
wants to merge 10 commits into from
Closed

[popups] Add data-closed style hook #882

wants to merge 10 commits into from

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Nov 27, 2024 •
edited
Loading

Adds hidden to popup components' State, and when true (i.e. hidden is applied with keepMounted) it has a data-closed style hook.

@atomiks atomiks added the core Infrastructure work going on behind the scenes label Nov 27, 2024
@mui-bot
Copy link

mui-bot commented Nov 27, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-882--base-ui.netlify.app/

Generated by 🚫 dangerJS against 58d25d5

@atomiks atomiks marked this pull request as ready for review November 28, 2024 06:56
michaldudak
michaldudak reviewed Nov 28, 2024
View reviewed changes
packages/react/src/alert-dialog/popup/AlertDialogPopup.tsx Outdated
@@ -90,10 +90,11 @@ const AlertDialogPopup = React.forwardRef(function AlertDialogPopup(
const state: AlertDialogPopup.State = React.useMemo(
() => ({
open,
mounted,
Copy link
Member

Choose a reason for hiding this comment

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

Not strictly related to these changes, but this now becomes public API - I feel like "mounted" doesn't describe the behavior very well, especially when keepMounted = true. "visible" could be a better option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps hidden to align with the DOM attribute that's being set?

Copy link
Contributor

Choose a reason for hiding this comment

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

visible sounds good too though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think hidden makes the most sense to align with the attr. visible adds yet another term to deal with

michaldudak
michaldudak reviewed Nov 29, 2024
View reviewed changes
packages/react/src/alert-dialog/popup/AlertDialogPopup.tsx Outdated
@@ -90,7 +90,7 @@ const AlertDialogPopup = React.forwardRef(function AlertDialogPopup(
const state: AlertDialogPopup.State = React.useMemo(
() => ({
open,
mounted,
hidden: !mounted,
Copy link
Member

Choose a reason for hiding this comment

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

Can we also use the same term internally?

atomiks
atomiks commented Nov 29, 2024
View reviewed changes
Copy link
Contributor Author

@atomiks atomiks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The hidden value returned from useAnchorPositioning() refers to it being hidden but still mounted, which is when the anchor is hidden, so you want the popup to be hidden too.

Our prop is called hideWhenDetached, and we don't expose a style hook, simply using a functional style instead. I'm wondering if we actually should always expose a style hook and remove hideWhenDetached, so they can just use CSS and allows for further customization.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2024
@atomiks
Copy link
Contributor Author

atomiks commented Dec 3, 2024

@vladmoroz thinking data-hidden (not transforming the hook name) makes more sense? Since data-closed seems like it should match open: false to match the other components' symmetric counterpart

@vladmoroz
Copy link
Contributor

Since data-closed seems like it should match open: false to match the other components' symmetric counterpart

Wait I don't follow, isn't it symmetric to open: false? Maybe an example would help

data-hidden would have made sense if the other one was data-visible

@atomiks
Copy link
Contributor Author

atomiks commented Dec 4, 2024

Wait I don't follow, isn't it symmetric to open: false? Maybe an example would help

This PR represents when it's fully unmounted. You were mentioning [hidden] isn't enough to target the relevant state since you can do hidden={false}. Maybe it should be data-closed (open: false state) AND data-hidden ([hidden] / mounted: false state)?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 4, 2024
@vladmoroz
Copy link
Contributor

Wait I don't follow, isn't it symmetric to open: false? Maybe an example would help

This PR represents when it's fully unmounted. You were mentioning [hidden] isn't enough to target the relevant state since you can do hidden={false}. Maybe it should be data-closed (open: false state) AND data-hidden ([hidden] / mounted: false state)?

I'd expect that just data-closed that corresponds to open: false, and then if they are force mounting or adding a manual hidden={false}, they can add their own styling attributes as needed

@colmtuite colmtuite added this to the 1.0.0-alpha.1 release milestone Dec 9, 2024
@atomiks
Copy link
Contributor Author

atomiks commented Dec 9, 2024

Going to close this and open a new one that matches the open: false boolean.

@atomiks atomiks closed this Dec 9, 2024
@atomiks atomiks deleted the feat/closed-style branch December 9, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak left review comments

@vladmoroz vladmoroz vladmoroz approved these changes

@colmtuite colmtuite colmtuite approved these changes

Assignees

@atomiks atomiks

Labels
core Infrastructure work going on behind the scenes PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
Base UI Alpha
Status: Done
Milestone
1.0.0-alpha.1 release
Development

Successfully merging this pull request may close these issues.
5 participants
@atomiks @mui-bot @vladmoroz @colmtuite @michaldudak