Toast component #148

vonwao · 2024-06-20T21:28:56Z

No description provided.

alisonhall · 2024-06-21T19:21:44Z

content/body/toast.php

+<!DOCTYPE html>
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Accessible Toast Notifications</title>
+    <link rel="stylesheet" href="toast.css">
+    <link rel="stylesheet" href="app.css">
+</head>
+<body>


These elements shouldn't be included in this /content/body/toast.php file. They would either already be automatically included when the page is initially compiled, or should be included in the /content/head/toast.php file.

alisonhall · 2024-06-21T19:24:13Z

content/body/toast.php

+            {
+                "label": "Create markup",
+                "highlight": "data-tooltip",
+                "notes": "Our script uses the <code>data-tooltip</code> attribute instead of the <code>title</code> attribute, since <strong>title</strong> is rendered by user agents by default and cannot be styled."


Isn't the data-tooltip just a attribute that the Enable custom tooltip component uses? I don't think that sites that are missing the Enable custom tooltip will know what to do with this attribute.

alisonhall · 2024-06-21T19:29:16Z

less/toast-demo.less

+@margin: 20px;
+@gap: 10px;
+@font-size: 16px;


Instead of pixels, you should be using rem or em units in order to make text resizing be possible. This is especially important for font sizes.

Suggested change

@margin: 20px;

@gap: 10px;

@font-size: 16px;

@margin: 1.25rem;

@gap: 0.625rem;

@font-size: 1rem;

alisonhall · 2024-06-21T19:30:07Z

less/toast.less

+@toast-padding: 10px;
+@toast-border-radius: 5px;
+@toast-box-shadow: 0 2px 4px rgba(0, 0, 0, 0.1);
+@toast-transition: all 0.5s ease-in-out;
+@toast-close-color: @toast-color;
+@toast-close-hover-color: #ff0000;
+@rack-background: rgba(241, 241, 241, 0.9);
+@rack-border: 1px solid #ddd;
+@rack-padding: 10px;
+@rack-width: 300px;
+@rack-max-height: 400px;


Again, consider using rem or em units instead of pixels whenever you can in this file.

For this and above, I am okay with just font-sizing and anything needed for that done in rems. For margins, padding, etc, it is okay to use px as long as it doesn't affect legibility (although it is not wrong to do so if it is more aesthetically pleasing).

zoltan-dulac · 2024-06-24T17:57:28Z

less/toast.less

+
+    &.bottom-right,
+    &.bottom-left,
+    &.bottom-center {


Most of Enable is written using BEM. The structure should be like:

.toast {

&__container {

&--bottom-right { }

}

&__rack {

}

(This is not exhaustive .. it's just an example)

concat.py

zoltan-dulac · 2024-07-03T16:17:30Z

concat.txt

What is this for?

zoltan-dulac · 2024-07-03T16:18:42Z

content/body/toast.php

+        <li>Toasts should support different levels of severity (normal, error, warning, success), each with a unique color for visual distinction.</li>
+    </ol>
+    <p>
+        Our implementation ensures that toasts are fully accessible and follow best practices for ARIA and keyboard interaction. When a toast appears, it is announced to screen readers. Toasts can be configured to stay visible for a set amount of time or until manually dismissed by the user. Additionally, all toasts are stored in a toast rack for future reference.


We may want to remove the set amount of time part. Let's discuss on your demo.

zoltan-dulac · 2024-07-03T16:21:00Z

css/app.css

This is not a LESS file. All CSS must be in LESS.

The name app.css is not specific. It should describe what it's used for. Is this for the demo, or the component?

I think this is an old issue, both less and css files are in repository, which I think is the correct pattern

zoltan-dulac · 2024-07-03T16:21:35Z

js/demos/toast.js

+import toastModule from '../modules/toast.js';
+
+const app = new (function () {
+    console.log('toast demo!!!');


Please remove. I am surprised this wasn't caught by unit testing.

zoltan-dulac · 2024-07-03T16:23:08Z

js/modules/es4/toast.js

+
+      // Initialize the toast rack
+      this.rackContent = document.getElementById('rackContent');
+      this.toggleRackButton = document.getElementById('toggleRackButton');


All CSS classes and IDs should be namespaced.

Use something like #enable-toast__rack-content and #enable-toast__rack-back-button.

zoltan-dulac · 2024-07-03T16:24:28Z

js/modules/es4/toast.js

+
+    // Update the visible toasts according to the max visible limit
+    updateVisibleToasts() {
+      console.log("Updating visible toasts");


Avoid keeping console.log() statements in your code. If they are absolutely necessary, use console.info(). I don't think this is, though.

zoltan-dulac · 2024-07-03T16:25:04Z

js/modules/es4/toast.js

+      this.visibleQueue.forEach((toast, index) => {
+        const toastElement = this.container.querySelector(`[data-id="${toast.id}"]`);
+        if (index < this.maxVisible) {
+          toastElement.classList.add('toast-visible');


CSS classes should enforce BEM:

enable-toast--visible

zoltan-dulac · 2024-07-03T16:25:53Z

js/modules/toast.js

Sorry -- all the comments for the ES4 code was meant for this file.

zoltan-dulac · 2024-07-03T16:27:45Z

less/toast-demo.less

+@font-size: 1rem; // 16px converted to rem
+
+// Controls styles
+.controls {


BEM should be used here. Let's fix this and the toast.less file. Discuss with me if you need guidance.

zoltan-dulac · 2024-07-11T12:38:42Z

less/toast.less

+
+    &.enable-toast__container--bottom-right,
+    &.enable-toast__container--bottom-left,
+    &.enable-toast__container--bottom-center {


You should just use

&__container--bottom-right, &__container--bottom-left, &__container--bottom-center { ... }

The point of BEM is ensure the components DOM structure can be changed completely and things will be styled the same. While this may not be a concern here, it is a concern for the selectors below.

What is relevant here is that BEM also flattens CSS specificity.

Let's talk when you have time.

vonwao added the WIP Work In Progress label Jun 20, 2024

alisonhall reviewed Jun 21, 2024

View reviewed changes

zoltan-dulac reviewed Jun 24, 2024

View reviewed changes

zoltan-dulac requested changes Jul 3, 2024

View reviewed changes

zoltan-dulac requested changes Jul 11, 2024

View reviewed changes

Otto Wachter added 24 commits October 9, 2024 10:54

add toast demo

2fdb241

body

8c444e9

improve toast examples, add toast modules

114db69

css files

db934db

add less files

28cf070

comments

1a4750e

small improvements/fixes

d12fe86

work

6080e13

add standalone html file to test toast

d612549

update units: px to rem

08b9fd8

toast.php

4d0723f

px to rem units

f0308a5

set role and add slight delay appending to DOM for screenreaders

ac75c8d

new example

b8d061f

delete

e1eb85f

update description for accessible toasts

733f190

namespace styles

2922425

update __container and __close-button BEM style

14b7e1c

fix/improve BEM usage

8bffff1

comment

1d4f140

revert

a8590c7

fix styles, debugging styles

79c82d8

remove debugging, fix flex direction

e46ecdd

es4 module

1486b94

vonwao force-pushed the toast-component branch from 8fc0d96 to 1486b94 Compare October 9, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toast component #148

Toast component #148

vonwao commented Jun 20, 2024

alisonhall Jun 21, 2024

alisonhall Jun 21, 2024

alisonhall Jun 21, 2024

alisonhall Jun 21, 2024

zoltan-dulac Jun 24, 2024

zoltan-dulac Jun 24, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

vonwao Jul 8, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 3, 2024

zoltan-dulac Jul 11, 2024

Toast component #148

Are you sure you want to change the base?

Toast component #148

Conversation

vonwao commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment