-
Notifications
You must be signed in to change notification settings - Fork 4
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
Close button API #48
Comments
The situation is similar to
|
IDL attributes for accessibility attributes apply nullable DOMString. |
I agree the API is a bit weird. It is favoring developer ergonomics over platform consistency at the moment. I am not sure we've landed on the right part of the spectrum there so it's good to have an issue to discuss. To expand more on why I think the current design is nice developer ergonomics, it basically allows developers to operate in one of two mindsets, without caring which: Current explainer<std-toast closebutton>...</std-toast>
<std-toast closebutton="Dismiss">...</std-toast> /* BOOLEAN MODE */
toast.closeButton = true;
toast.closeButton = false;
// This works even if other parts of the code set closeButton to a string
if (toast.closeButton) { ... }
showToast("Message", { closeButton: true });
/* STRING MODE */
toast.closeButton = "Dismiss";
if (toast.closeButton === "Dismiss") { ... }
showToast("Message", { closeButton: "Dismiss" }); A couple alternatives have been mentioned in the thread so far: Nullable DOMStringHere if the attribute is missing, <std-toast closebutton>...</std-toast>
<std-toast closebutton="Dismiss">...</std-toast> toast.closeButton = ""; // set default close button
toast.closeButton = null; // remove close button
toast.closeButton = "Dismiss";
if (toast.closeButton !== null) { ... }
showToast("Message", { closeButton: "" });
showToast("Message", { closeButton: "Dismiss" }); (Note: I assume we keep the showToast option the same as the property, but we could also explore diverging them. I'll keep it simple for now.) Two separate attributesThis looks like: <std-toast closeable>...</std-toast>
<std-toast closeable closebutton="Dismiss">...</std-toast>
<!-- does nothing useful -->
<std-toast closebutton="Dismiss">...</std-toast> // set default close button
toast.closeable = true;
// set customized close button
toast.closeable = true;
toast.closeButton = "Dismiss";
// remove close button
toast.closeable = false;
// don't do this, it will make a close button with no text/the string "null" inside:
toast.closeButton = "";
toast.closeButton = null;
if (toast.closable) { ... }
// We could probably make closeable: true implicit
// if closeButton is specified for showToast():
showToast("Message", { closable: true });
showToast("Message", { closeButton: "Dismiss" }); Thoughts welcome! |
…se functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug:972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed
…se functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Jack Steinberg <jacksteinberg@chromium.org> Cr-Commit-Position: refs/heads/master@{#678530}
…se functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Jack Steinberg <jacksteinberg@chromium.org> Cr-Commit-Position: refs/heads/master@{#678530}
…se functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Jack Steinberg <jacksteinberg@chromium.org> Cr-Commit-Position: refs/heads/master@{#678530}
There's also <std-toast>
Content
<img src="..." slot=closebutton>
</std-toast> |
…st closebutton and attach close functionality, a=testonly Automatic update from web-platform-tests Create an attribute and property for toast closebutton and attach close functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Jack Steinberg <jacksteinberg@chromium.org> Cr-Commit-Position: refs/heads/master@{#678530} -- wpt-commits: bd0b2bff5361391e8c25949aa40f3167dd82d572 wpt-pr: 17869
…st closebutton and attach close functionality, a=testonly Automatic update from web-platform-tests Create an attribute and property for toast closebutton and attach close functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Jack Steinberg <jacksteinberg@chromium.org> Cr-Commit-Position: refs/heads/master@{#678530} -- wpt-commits: bd0b2bff5361391e8c25949aa40f3167dd82d572 wpt-pr: 17869
…se functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue web-platform-tests#48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergal@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Jack Steinberg <jacksteinberg@chromium.org> Cr-Commit-Position: refs/heads/master@{#678530}
…st closebutton and attach close functionality, a=testonly Automatic update from web-platform-tests Create an attribute and property for toast closebutton and attach close functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergalchromium.org> Reviewed-by: Kent Tamura <tkentchromium.org> Commit-Queue: Jack Steinberg <jacksteinbergchromium.org> Cr-Commit-Position: refs/heads/master{#678530} -- wpt-commits: bd0b2bff5361391e8c25949aa40f3167dd82d572 wpt-pr: 17869 UltraBlame original commit: 8930d78b9176fc46cc05bc4f0f3f1465128d1da1
…st closebutton and attach close functionality, a=testonly Automatic update from web-platform-tests Create an attribute and property for toast closebutton and attach close functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergalchromium.org> Reviewed-by: Kent Tamura <tkentchromium.org> Commit-Queue: Jack Steinberg <jacksteinbergchromium.org> Cr-Commit-Position: refs/heads/master{#678530} -- wpt-commits: bd0b2bff5361391e8c25949aa40f3167dd82d572 wpt-pr: 17869 UltraBlame original commit: 8930d78b9176fc46cc05bc4f0f3f1465128d1da1
…st closebutton and attach close functionality, a=testonly Automatic update from web-platform-tests Create an attribute and property for toast closebutton and attach close functionality This change sets the groundwork for the toast closebutton, by adding an attribute to set the closebutton content, creating a closeButton property to reflect that attribute, and adding an event listener to close the toast when the closebutton is pressed. This change implements the close button API described in (https://github.com/jackbsteinberg/std-toast/blob/eec7728f7082a897d777181ac07b0448062ffca5/README.md), and is the subject of debate on issue #48 on the explainer here jackbsteinberg/std-toast#48. Future CLs will add the closeButton option for showToast (https://github.com/jackbsteinberg/std-toast/tree/master#showtoastmessage-options) Link: https://chromium-review.googlesource.com/c/chromium/src/+/1706453 Bug: 972945 Change-Id: I5d6c3055791a9f93ef414e575e128c61e2a944ed Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699269 Reviewed-by: Fergal Daly <fergalchromium.org> Reviewed-by: Kent Tamura <tkentchromium.org> Commit-Queue: Jack Steinberg <jacksteinbergchromium.org> Cr-Commit-Position: refs/heads/master{#678530} -- wpt-commits: bd0b2bff5361391e8c25949aa40f3167dd82d572 wpt-pr: 17869 UltraBlame original commit: 8930d78b9176fc46cc05bc4f0f3f1465128d1da1
Maybe this should just be in #47 but it's not just about naming.
Is a text attribute the right approach? Should we support e.g. an image for a close button? (Edit, so e.g. a slotted child).
Also, the spec for the value of the
closeButton
attribute seems a bit complicated, it's trying to combine 2 things into 1and I'm not sure that's a great idea. We could consider having a
closeable
boolean property and aclosebutton
that returns a string. I don't know if that's really better but a property that can true/false/string seems very odd to me.The text was updated successfully, but these errors were encountered: