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

Dialog: Add defaultVisible property #6772

Open
jpranays opened this issue Jun 19, 2024 · 13 comments · May be fixed by #6770
Open

Dialog: Add defaultVisible property #6772

jpranays opened this issue Jun 19, 2024 · 13 comments · May be fixed by #6770
Assignees
Labels
Status: Discussion Issue or pull request needs to be discussed by Core Team Type: New Feature Issue contains a new feature or new component request

Comments

@jpranays
Copy link

jpranays commented Jun 19, 2024

Use Case

Imagine a scenario where a dialog needs to be immediately visible upon page load, such as for displaying an important announcement or a welcoming message. Here, the parent component doesn't require ongoing control over the visibility state post-initial render.

The defaultVisible prop is a boolean that, when set to true, ensures that the Dialog is displayed immediately upon mounting. Conversely, setting this prop to false or omitting it entirely will maintain the default behavior, where the Dialog is hidden initially and requires a trigger to become visible.

Changes

  • Added defaultVisible prop to the Dialog component's API.
  • Updated the Dialog component's internal state management to account for the defaultVisible prop.
  • Ensured backward compatibility by defaulting the defaultVisible prop to false.

Usage

<Dialog defaultVisible={true}>
    <!-- Dialog content goes here -->
</Dialog>
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Jun 19, 2024
@melloware melloware changed the title Add defaultVisible Prop to Dialog Component Dialog: Add defaultVisible property Jun 19, 2024
@melloware melloware added Type: New Feature Issue contains a new feature or new component request Status: Discussion Issue or pull request needs to be discussed by Core Team and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Jun 19, 2024
@melloware melloware linked a pull request Jun 19, 2024 that will close this issue
@sja-cslab
Copy link
Contributor

What about:

const [myVisibleState, setMyVisibleState] = useState(false);
<Dialog defaultVisible={true} visible={myVisibleState}>
    <!-- Dialog content goes here -->
</Dialog>

Is it visible or not? If inner state change what happens to my state? thats just weird

@jpranays
Copy link
Author

The scenario you described might sound as a potential issue with having both defaultVisible and visible props simultaneously. But to clarify further The defaultVisible prop is intended for use cases where the dialog needs to be shown immediately upon page load without requiring further state management in the parent component.
Even if user tries to achieve above scenario through its own state logic, then it wont affect the default working behaviour of Dilaog, I tested.

@sja-cslab
Copy link
Contributor

So you say defaultVisible > visible?
That sounds incorrect. Default means "if not set" normally. So if visible would be set, default should be ignored.

I see your point but personally think that this causes more conflicts in logic than it has any benefits over a visible state and onHide handling.

@jpranays
Copy link
Author

I appreciate your perspective. The defaultVisible prop is designed for developers to avoid additional state management in the parent component for such cases.

The intent is to offer a straightforward solution for scenarios like announcements or welcome messages, where managing state isn't necessary beyond the initial render. This can be especially beneficial when multiple dialogs are present on a page, reducing the need for separate state management for each dialog.

@sja-cslab
Copy link
Contributor

What about a new sub-component to avoid those confusing defaultVisible/visible things?

We could let it inhert from Dialog and eleminate properties that aren't required to keep complexity low for users.

@melloware
Copy link
Member

I am all for keeping complexity low and having both defaultVisible and visible properties will confuse users.

@sja-cslab
Copy link
Contributor

sja-cslab commented Jun 20, 2024

@melloware What about the idea of a new Component? I could imagine something like <Banner title="Welcome" content="Thats a new component that will be visible on render till you press that X in upper right corner" />

You see such things often on Shops. "it's our 10th Birthday - 10% on all items in Stock" or something.

@melloware
Copy link
Member

I guess what I am struggling with is visible={true} will do exactly that already and display upon load.

I have seen components you are talking about before in Primefaces JSF its called a Notification Bar. https://www.primefaces.org/showcase/ui/panel/notificationBar.xhtml?jfwid=7b423

@sja-cslab
Copy link
Contributor

sja-cslab commented Jun 20, 2024

I guess what I am struggling with is visible={true} will do exactly that already and display upon load.

I have seen components you are talking about before in Primefaces JSF its called a Notification Bar. https://www.primefaces.org/showcase/ui/panel/notificationBar.xhtml?jfwid=7b423

That component looks like something that could do what OP want.

However I was more the direction of something like the following.

image

You see this purple-like banner with the timer on it on top of the page? That what I can think of.
A little bit content, a close button and thats it.

@jpranays
Copy link
Author

I am all for keeping complexity low and having both defaultVisible and visible properties will confuse users.

I understand the concern about complexity. However, users are unlikely to pass both defaultVisible and visible at the same time if their goal is to handle only the initial render. To maintain the component's standard and provide convenience, the defaultVisible prop could be a beneficial addition.

@melloware
Copy link
Member

Yep that PrimeFaces JSF component is exactly like your screenshot about how we use it at one of my clients. It can be styled any way you want. I have seen other component libraries sometimes call it a Drawer.

@sja-cslab
Copy link
Contributor

Yep that PrimeFaces JSF component is exactly like your screenshot about how we use it at one of my clients. It can be styled any way you want. I have seen other component libraries sometimes call it a Drawer.

Good to know - was unsure about the name of it.
However PrimeReact does not have any equivalent correct?

@melloware
Copy link
Member

It does not Primefaces JSF has been around since 2009 so it has a lot of components the other libs don't but also because PrimeTek chose to whittle the list down in the JS libs to only critical components they want to support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Discussion Issue or pull request needs to be discussed by Core Team Type: New Feature Issue contains a new feature or new component request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants