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

[invokers] Dialog's toggle needs a better name #956

Closed
keithamus opened this issue Nov 16, 2023 · 4 comments
Closed

[invokers] Dialog's toggle needs a better name #956

keithamus opened this issue Nov 16, 2023 · 4 comments
Labels
invokers needs edits This is ready for edits to be made stale

Comments

@keithamus
Copy link
Collaborator

Invokers targeting a <dialog> can have invokeaction=toggle, which will open and close the dialog as non-modal.

toggle is a little ambiguous, especially as all other names are very clear in their action, togglePopover, playpause, etc.

Given whatwg/html#9376 (Should we deprecate Dialog show()?) perhaps we can simply not implement this?

If we are to implement this, it needs a clearer name. Some bad ideas to start the bidding:

  • toggleNonModal
  • toggleOpen
  • toggleShow
@lukewarlow
Copy link
Collaborator

I vote "don't implement". ToggleOpen isn't a perfect representation of what it does (given focusing) so I would vote against that.

@lukewarlow lukewarlow added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Dec 1, 2023
@css-meeting-bot
Copy link

The Open UI Community Group just discussed [invokers] Dialog's `toggle` needs a better name, and agreed to the following:

  • RESOLVED: Defer `toggle` and `show`, remove the explicit `toggleModal` string, and keep `auto` behaviours as is.
The full IRC log of that discussion <masonf> q?
<hdv> keithamus: the toggle method may be ambiguous… the others are unambiguous to the target element, like showPopover
<Luke> q+
<masonf> q+
<hdv> keithamus: for showing and hiding a dialog for an invoke button
<masonf> q+ sasha
<hdv> Luke: details is ambiguous as well
<hdv> Luke: having said that, details isn't in the v1
<hdv> Luke: would say don't implement show() or toggle() in the initial version
<hdv> s/show()/show
<hdv> s/toggle()/toggle
<masonf> ack luke
<hdv> Luke: so if we decide to keep non modal dialog in the future for whatever reason ew could
<hdv> Luke: most people will likely use a modal dialog or a popover
<hdv> masonf: can we punt the whole toggle thing… there is no such API on dialogs now can we just leave showmodal and close and auto?
<hdv> masonf: I'm still questioning… if you're toggling… the use case for toggle on a modal doens't make sense to me
<masonf> ack mason
<hdv> keithamus: use case for toggle on modal is the auto action
<hdv> keithamus: do we have a name mapping for the auto action other than auto?
<hdv> keithamus: another meta question around this… do we intentionally want tot disincentivise showing non modal dialog from dialog?
<hdv> s/tot/to
<hdv> masonf: don't think we should actively disincentivise anything
<hdv> masonf: on most parts of the platform auto is magic
<keithamus> q+
<hdv> masonf: I don't think there's a problem with auto not mapping to a string value you can provide
<masonf> ack sasha
<hdv> sasha: I can see the reference to existing solutions here
<hdv> sasha: I think we need to align if it fits with the old semantics… it looks from this convo that it does, only want to display or hide
<hdv> sasha: not sure about the toggle as a part of the flow in the page… they usually work together with the whole page… what would happen next after triggering the dialog?
<masonf> q?
<hdv> sasha: when we do the proposal it would be nice to outline generic use cases and scope
<masonf> ack keith
<hdv> keithamus: one reason not to want auto this way is that auto is ambiguous depending on the target element
<hdv> keithamus: I didn't understand your last point sasha?
<hdv> sasha: usually they are triggering the visibility associated with the next steps in the user flow
<hdv> sasha: so if you toggle a dialog and dialog has an extra attribute, they would not just toggle visibility of that one, but also toggle details with open attribute after the close of this dialog
<hdv> keithamus: I think dialogs already have some of the behaviour that you describe
<hdv> masonf: there are still events, close and cancel on dailogs, that could be used to trigger the next
<hdv> sasha: am thinking as declarative
<masonf> q?
<scotto_> q+
<hdv> luke: on toggle button you have buttons that are literally toggles… but on a modal you'd have buttons that only do either, just a single action
<masonf> q+
<hdv> Luke: so I think folks would either go auto or toggle mode
<keithamus> q+
<hdv> scotto_: +1 what you mentioned Mason, Keith and Luke both know I've had a weirdness to toggling modals, you can't do that from a single button, outside of a weird use case of someone forcing a dialog to be open
<hdv> scotto_: so we probably don't need to add a toggle attribute for that
<hdv> ack scotto_
<hdv> masonf: not having auto for dialogs feels kind of funny
<hdv> masonf: so the spec would have to say if it is a popover, do this, if it's a dialog, do that… so maybe auto doesn't need to mean toggle, it can mean use as modal?
<masonf> ack masonf
<hdv> Luke: we don't have auto for everything, eg for select there isn't an auto
<hdv> masonf: shoulnd't there be?
<hdv> Luke: if we add togglepicker or closepicker… if we have auto now we couldn't add those in the future
<hdv> masonf: I'd argue show is a good default there for same reason as dialog
<hdv> Luke: I see the point
<hdv> q?
<masonf> q+ charlie
<hdv> keithamus: the intend of auto toggling the modal is thatyou don't have to specify the invokeaction attribute
<hdv> keithamus: so inside the dialog not specifying the attribute would do the sensible thing, close the attribute… it saves devs from writing the attribute, browser could be clever enough to do the right thing
<hdv> keithamus: maybe the correct thing to do here… remove the aliases to auto, and auto will, as spec says today, if target el is popover, it will toggle as a popover, if the element is a dialog, it will show as modal unless dialog is open then it willl close it
<masonf> q?
<masonf> ack keith
<masonf> ack charlie
<hdv> charlie: my concern with going ahead with auto, because we don't know the full list of invokee elements right now, but want to support it for some other than popover and dialog, we probably want to keep it consistent
<hdv> charlie: this may give us confusing use cases
<masonf> q+ sasha
<hdv> charlie: my vote would be to defer auto until we have a better idea what would be included in invokers
<hdv> masonf: so you say it would feel weird for auto to do fundamentally different things for different elements so it is fair to wait until we know more?
<hdv> charlie: yes
<hdv> masonf: would cause for everyone to always add both attributes
<hdv> Luke: is the case for popover currently too
<hdv> masonf: it is nice not to have to add extra attributes
<hdv> sasha: some buttons could have three states, sounds like these could be subject to the mode, would be nice to provide those states
<masonf> q?
<hdv> masonf: top layer is pretty carefully controlled so you can't change popover to dialog without closing it first
<keithamus> PROPOSED RESOLUTION: Defer toggle, and remove `auto` aliases (such as `toggleModal`).
<masonf> ack sasha
<hdv> masonf: maybe to clarify: do you mean continue supporting auto but not having a mapping?
<hdv> keithamus: yes
<hdv> Luke: you intend to keep togglePopover as a standalone?
<hdv> masonf: that's pretty well defined, maps to a function
<keithamus> Proposed Solution: Defer `toggle`, remove the explicit `toggleModal` string, and keep `auto` behaviours as is.
<masonf> +1
<scotto_> +1
<hdv> +1
<hdv> Luke: can I clarify does this include show?
<hdv> masonf: would make sense to keep?
<keithamus> Proposed Resolution: Defer `toggle` and `show`, remove the explicit `toggleModal` string, and keep `auto` behaviours as is.
<masonf> +1
<Luke> +1
<hdv> +2
<keithamus> Resolved: Defer `toggle` and `show`, remove the explicit `toggleModal` string, and keep `auto` behaviours as is.
<hdv> (for British spelling)
<keithamus> RESOLVED: Defer `toggle` and `show`, remove the explicit `toggleModal` string, and keep `auto` behaviours as is.

@sashafirsov
Copy link

The separation of concerns IMO needs separate attributes:

  • ther aspect of visibility, i.e. switch the shown/hidden state
  • the mode definition of how the dialog would be shown(modal/non-modal/inline)
  • what should happen after the dialog completed(triggered)

It is important as provide each functionality separately and do not mix those. As the imperative as declarative syntax for all. In this case declarative can be limited by allocation of DIALOG attributes.

Open

open as name for visibility to match the other HTML elements like in details element

Toggle/Trigger

triggering the next UI on the toggle as pointer to the what should happen to element refered say by for attribute. Scenario:

  • trigger the current dialog by setting open=true attribute
  • get the ref element by for attribute
  • get the target state of ref element from toggle attribute
  • open/close the targer element, pass the focus

Mode

the modal vs popover vs inline is rather the subject for mode attribute of Dialog.

@lukewarlow lukewarlow added needs edits This is ready for edits to be made and removed agenda+ Use this label if you'd like the topic to be added to the meeting agenda labels Dec 7, 2023
Copy link

github-actions bot commented Jun 5, 2024

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invokers needs edits This is ready for edits to be made stale
Projects
None yet
Development

No branches or pull requests

4 participants