Allow messages to be dismissable by ENT8R · Pull Request #64 · rugk/offline-qr-code

ENT8R · 2018-04-30T10:54:05Z

This PR fixes #11

rugk

Basically nice change… also good you made it as an extra class.

However, not all messages should be dismissable automatically. Basically, it would be great if I one could pass a simple (boolean) parameter (with a default value) to the function of the message handler.

E.g. the errors shown when the qr code cannot be generated must not be hidden.
But in the same popup the tips shown as info messages must be dismissable.
I think currently that is not really possible, is not it?
(If you think otherwise, just try to convince me. 😉 )

rugk · 2018-04-30T14:14:44Z

+            elMessage.appendChild(iconDismiss);
+
+            iconDismiss.addEventListener("click", () => {
+                elMessage.classList.add("invisible");


I think the event listener should send an event parameter, so better use that for hiding it. IMHO the scope is then much clearer. 😄

rugk · 2018-04-30T14:29:27Z

+.icon-dismiss {
+  background-image: url('/common/img/close.svg');
+  position: absolute;
+  right: 10px;


this "margin" should be 4px, see https://design.firefox.com/photon/components/message-bars.html#sizes-and-grid
but remember that the button/icon itself also already has a margin…

Correction: See #64 (comment)

rugk · 2018-04-30T14:29:43Z

 .success::before {
  background-image: url('/common/img/check.svg');
 }
+.icon-dismiss {


There are still some styles missing for hovering e.g.
AFAIK the correct styles are described as a ghost button, but this site also shows some screenshots (when not hovered).

(I don't really know whether it should have a transparent background or really that grey one as specified)

So I've asked them about this.

rugk · 2018-04-30T14:37:28Z

 		<meta name="viewport" content="width=device-width, initial-scale=1.0">

-		<link rel="stylesheet" href="../common/common.css">
+		<link rel="stylesheet" href="common/common.css">


I doubt that works…

You can of course make the path an absolute one.

But the path for the JS file does look like this common/common.js...

Well I thought just make it explicitly relative (like/commons/…) then, but well… yeah. Just strange why it worked then in the past.

Well… basically does not really matter.

ENT8R · 2018-04-30T15:51:04Z

I think currently that is not really possible, is not it?

An hour ago it was not possible, but now it should work without any problems 😏

rugk · 2018-04-30T16:17:00Z

        }

-        if (elMessage.classList.contains("dismissible")) {
+        if (typeof args[1] === "boolean" && args[1]) {


This needs to be adjusted in the JSDoc, also in all functions, which call showMessage.

Also better use a const here for code style reasons (who knows what arg1 is, otherwise?). (or, better, shift as I've done before…)

Also this codes likely needs to be moved some lines up, because in line 515 I already use all other arguments for the translation of the message – so this would include the boolean…

rugk · 2018-04-30T16:34:20Z

-        if (elMessage.classList.contains("dismissible")) {
+        if (typeof args[1] === "boolean" && args[1]) {
            // add an icon which dismisses the message if clicked
            const iconDismiss = document.createElement("span");


Okay, it creates this button – but it does not remove it (or at least make it invisible) when another message without "dismissable" set to true is shown.

As such maybe generally do it in another way: Add the HTML to the html file and only hide/unhide it when needed. This has two other advantages:

if a message may never get a dismissable button, because of the way it is shown or so, it could just not be added to the HTML (such a case would be the Popup QR code error, IMHO). So in the JS you need to check for the existence of that element!

That would also make customization easier.

But all contents of the message box are replaced by setting elMessage.textContent = localizedString; here...

rugk · 2018-04-30T17:01:51Z

I tested it out and the right margin at border looks strange. I also made a thought mistake here reading the Photon guide. The 4px "margin" to the right is actually the padding of the message box, which is already applied in the style – just because you use absolute positioning you are overwriting it there, as it seems.
The margin around the icon is an additional margin just around the icon, so in the end, we have 8px to the right. THis also looks okay, then:

However, it would be preferably if you could not use absolute positioning there anyway. Maybe something like this. Note the element is already in a flex, so you can happily use Flexbox there.

Generally, BTW, you can test out whether an auto-width (not 100%) looks better. I actually have no strong option on this (and Photon allows both styles based on vague "small/high width" definition), but currently I feel the "close" (dismiss) button is a bit too far away from the actual message.

rugk · 2018-04-30T17:06:48Z

Also another problem: Based on the screenshots I think the hover area needs to be actually include the 4px margin around the icon, i.e. be large enough, so that only the 4px padding of the message container is around the clickable area.

Currently the clickable area is just too small:

rugk · 2018-04-30T17:14:35Z

+.icon-dismiss:active {
+  background-color: rgba(12, 12, 13, 0.2);
+}
+.icon-dismiss:focus {


I just tested around a bit and the element is not even focusable by default. You need to add tabindex="1" to make it actually focusable. Then we can see the design.

Also, I think here yet again, a rounded border should be used (4px), although not explicitly specified in the Photon guide, but the screenshots look like this.

As for the rounded border, I have now complained here.

Isn't this documented here? There they say "Corner Radius: 2px"

Hmm, only for the default (big) button as far as I see, nothing is said for ghost buttons.
But yes, maybe use 2px, because if the other buttons have two pixels then this one may have, too.

rugk

Oh and another thing: Make the close button actually accessible, i.e. add some aria attribute, I think.

ENT8R · 2018-05-02T12:46:34Z

Generally, BTW, you can test out whether an auto-width (not 100%) looks better

IMO this looks strange... I would also only search for a dismiss icon in the top right corner.

ENT8R · 2018-05-02T14:53:28Z

I think the hover area needs to be actually include the 4px margin around the icon, i.e. be large enough, so that only the 4px padding of the message container is around the clickable area.

Do you have an idea how this can be achieved?

rugk · 2018-05-03T20:35:12Z

I'll have a look, i.e. I take it over…

rugk · 2018-05-03T21:48:21Z

TODO/ideas:

maybe still move HTML code to all single items, so it does not need to be copied in JS? Would be useful if there are different messages with different (styles of) close buttons…
maybe do not use border-box?
maybe some fancy hide animation, so it does not look so abrupt when hidden?

rugk · 2018-05-05T14:51:13Z

Animation is done:

rugk · 2018-05-06T19:13:14Z

So I moved the dismiss icon into the HTML source code, and basically that resulted in a larger refactoring than I thought. I also added some rules, which I did not thought of, to the contributing doc.

rugk · 2018-05-06T20:55:20Z

~~As for box-sizing I have no idea why it actually works…~~

For some very strange reason box-sizing is actually set to border-box by default by some Firefox internal CSS:

That is contrary of what it is for websites, where content-box is the default.

So basically I think, @ENT8R, that was your issue when trying to use padding for the 4px difference there… Ugh…

As said here:

You might think of it this way: with box-sizing: border-box the padding and border press their way inside the box rather than expand the box.

Well… yeah, but unfortunately this is not what we want here, because padding with non-fixed height etc. is then totally useless as it is just calculated into the height and thus does not apply.

Now I understand why it is useful though. When I reset the box-sizing on the message-box, the 100% width is too large and it does not actually fit into the window, so it shows some scroll bars:

As I think the top and bottom padding is actually not intended, I changed the CSS a bit to match that.

…wtf…

I tried around it even more and the box-sizing: border-box; is maybe actually possible for two effects:

that the QR code sometimes is cut off from the bottom. (Sometimes QR code is overlapped by bottom textarea #66)
that my whole resizing to smaller sizes works, because you have still some pixel you can re-size (the padding more or less) without getting limited at some minimal size where it does not make the popup 5px smaller…

Sorry for the random thoughts, but this thing just totally caught me in surprise… 😵 It seems to cause both good and bad as you can see from the examples. Padding can go anywhere and be ignored – or even worse – minimize the content you actually want to have a fixed width, but you can have other good things…

rugk · 2018-05-06T21:09:13Z

So I created a thread, because it should not happen that I have to discover it this way. That fundamental change is just not-documented anywhere.

rugk · 2018-05-10T18:24:50Z

@ENT8R BTW, just wanted to make you aware of one issue that slipped though…

See e4af72d

Currently you just have to always add the basic attribute to make the element being recognized by the i18n translation module; and the attribute to change needs to be prefixed. (Of course, feel free to improve that behavior. That would be one step for #46. Note that prefixing is deliberate, so we can have a no-JS fallback.)

ENT8R · 2018-05-10T18:28:18Z

@ENT8R BTW, just wanted to make you aware of one issue that slipped though…

Oh... Thank you for correcting this!

Allow messages to be dismissable

1620d57

rugk requested changes Apr 30, 2018

View reviewed changes

Adjust styles, refactor code to dismiss the message by a parameter

5555d9e

Make code better readable

3b355c8

rugk requested changes Apr 30, 2018

View reviewed changes

Add some more improvements for the dismiss message feature

4edd1b5

Merge branch 'master' into dismiss-message

bd4215c

rugk self-assigned this May 3, 2018

rugk added 2 commits May 3, 2018 23:25

Adjust param handling so dismiss boolean is second param

b615328

Adjust CSS to include padding inside of image

970c570

Add transition when hiding message

e693df6

rugk added 2 commits May 5, 2018 17:13

Fix hiding of element

437b0d3

Refactor message dismissing

48f5af9

Add box-sizing adjustments/notes

1cbe131

rugk approved these changes May 6, 2018

View reviewed changes

rugk merged commit 1cbe131 into rugk:master May 6, 2018

ENT8R deleted the dismiss-message branch May 7, 2018 04:29

Conversation

ENT8R commented Apr 30, 2018

Uh oh!

rugk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ENT8R commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rugk commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rugk commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rugk left a comment

Choose a reason for hiding this comment

Uh oh!

ENT8R commented May 2, 2018

Uh oh!

ENT8R commented May 2, 2018

Uh oh!

rugk commented May 3, 2018

Uh oh!

rugk commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rugk commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rugk commented May 6, 2018

Uh oh!

rugk commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rugk commented May 6, 2018

Uh oh!

rugk commented May 10, 2018

Uh oh!

ENT8R commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rugk commented Apr 30, 2018 •

edited

Loading

rugk commented May 3, 2018 •

edited

Loading

rugk commented May 5, 2018 •

edited

Loading

rugk commented May 6, 2018 •

edited

Loading