fixes #467 add data error alert notice #478

spasovski · 2020-05-07T21:34:11Z

Do localStorage.clear() in your console to see again after dismissing.

Not super happy with the text copy and possibly color scheme so feel free to suggest better.

Updated with John's copy:

openjck · 2020-05-07T21:54:06Z

src/components/controls/AlertNotice.svelte

+</style>
+
+{#if alertVisible}
+  <div class="alert-notice" transition:fade={{easing: cubicOut}}>


65:46 error A space is required after '{' object-curly-spacing 65:63 error A space is required before '}' object-curly-spacing

And yay fade!

openjck · 2020-05-07T21:57:26Z

src/components/controls/AlertNotice.svelte

+  // Required: Which localStorage key to check whether this notice has been dismissed.
+  export let toggleKey = '';
+
+  $: alertVisible = window.localStorage.getItem(KEY_PREFIX + toggleKey) !== 'true';


In Slack, Rob Miller mentioned that he'd like the message to reappear daily if it's not too much engineering effort, or if that would be too much engineering effort, to use sessionStorage instead.

What does the $: reactivity do in this case? It doesn't seem that manually altering localStorage has any immediate effect.

Good catch, just an artifact of some earlier testing.

Just a remnant of an earlier approach, and yep I added the check against today

openjck · 2020-05-07T21:59:11Z

src/components/controls/AlertNotice.svelte

+
+  // So we don't pollute the entire LS namespace.
+  const KEY_PREFIX = 'alertNotice-';
+  export let dismissText = 'dismiss'; // Label of 'dismiss' button.


For the sake of consistency, I'd suggest putting all of the props just below the imports.

openjck · 2020-05-07T22:04:55Z

src/components/controls/AlertNotice.svelte

+    border-radius: var(--border-radius-base);
+    padding: var(--space-2x);
+    align-items: center;
+    grid-gap: var(--space-3x);


This doesn't do anything in display: flex, does it?

Oh I guess it does. Guess I need to brush up.

openjck · 2020-05-07T22:06:57Z

src/components/controls/AlertNotice.svelte

+    padding: var(--space-2x);
+    align-items: center;
+    grid-gap: var(--space-3x);
+    width: fit-content;


Huh. I've never used this!

Can this be used here? Firefox says it's an invalid value.

Yea another remnant of an earlier approach. Good catch.

openjck · 2020-05-07T22:09:00Z

src/routing/wrappers/Layout.svelte

@@ -71,4 +72,7 @@
      <Footer />
    </ContentFooter>
  </Content>
+  <AlertNotice toggleKey="dataErrorsWarning">
+    <p>The staging and development instances of GLAM are likely to have data issues and UI bugs.</p>


I actually think this copy is great. The only thing I'd suggest is maybe being more specific about "data issues" if we can.

I suppose if someone is on dev, they may not care about stage, and vice versa. Maybe something like this.

"This dashboard is under active development. Bugs and [data accuracy issues] may exist."

But I'd defer to Rob for the final copy.

openjck · 2020-05-07T22:13:25Z

src/components/controls/AlertNotice.svelte

+  }
+
+  .alert-notice-action button {
+    padding: var(--space-base);


We're way ahead of Elon!

we really should make this --space-1x across the board.

openjck · 2020-05-07T22:21:55Z

src/components/controls/AlertNotice.svelte

+
+<style>
+  .alert-notice {
+    position: absolute;


Maybe position: sticky instead, so that it remains at the bottom-left when the page scrolls?

Seems a little intrusive but I'll see what the consensus is. Easy to change.

openjck · 2020-05-07T22:24:45Z

src/components/controls/AlertNotice.svelte

+<style>
+  .alert-notice {
+    position: absolute;
+    left: var(--space-4x);


For some reason, it feels more natural to me on the right. I want to say it's because of some common UI convention or something, but it may just be personal preference.

openjck

Added some comments

hamilton

Looking great @spasovski. Thanks for getting this together so quickly.

I'd love to see a dismiss icon as well. If only a new icon set could be easily and effortlessly applied, gosh. (maybe the Close icon). These icons don't require any other stylesheet so they can be used without postcss, so it should be as easy as npm installing the package and then importing the one you want to use. I think the story shows all the icons + the names if you want to peruse.
I'd like to see a slight shadow to the element, if possible, to bring it a bit off the page. I think we might have some barely-used box-shadow variables that could be applied.
made a suggestion for the copy that gives more context.

hamilton · 2020-05-07T21:51:36Z

src/components/controls/AlertNotice.svelte

+</style>
+
+{#if alertVisible}
+  <div class="alert-notice" transition:fade={{easing: cubicOut}}>


I might suggest transition:fly with a y: 10 or so and a duration of maybe 200?

(I appreciate the instinct to apply a transition!)

hamilton · 2020-05-08T14:27:58Z

src/routing/wrappers/Layout.svelte

@@ -71,4 +72,7 @@
      <Footer />
    </ContentFooter>
  </Content>
+  <AlertNotice toggleKey="dataErrorsWarning">
+    <p>This dashboard is under active development. Bugs and data accuracy issues may exist.</p>


Suggested change

<p>This dashboard is under active development. Bugs and data accuracy issues may exist.</p>

<p>Thank you for testing the GLAM prototype!

This tool is still in active development. UX bugs and data issues may exist.

Help us make it suit your needs by directing questions or feedback to the #glam Slack channel.</p>

hamilton · 2020-05-08T14:28:20Z

src/components/controls/AlertNotice.svelte

+  }
+
+  .alert-notice-action button {
+    padding: var(--space-base);


we really should make this --space-1x across the board.

spasovski requested review from hamilton and openjck May 7, 2020 21:34

openjck reviewed May 7, 2020

View reviewed changes

hamilton suggested changes May 8, 2020

View reviewed changes

spasovski added 3 commits May 8, 2020 12:52

fixes #467 add data error alert notice

def17bf

address initial review concerns

5ef2e37

review comments and new close button

c7eb289

spasovski force-pushed the data-notice branch from 9f4344a to c7eb289 Compare May 8, 2020 17:54

more close btn padding, link to glam channel

84a76b5

spasovski merged commit 5b56207 into master May 8, 2020

spasovski deleted the data-notice branch May 8, 2020 19:03

hamilton added this to Done in 2020.5.0 May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #467 add data error alert notice #478

fixes #467 add data error alert notice #478

spasovski commented May 7, 2020 •

edited

Loading

openjck May 7, 2020

openjck May 7, 2020

openjck May 7, 2020

openjck May 7, 2020

spasovski May 8, 2020

spasovski May 8, 2020

openjck May 7, 2020

openjck May 7, 2020

openjck May 7, 2020

openjck May 7, 2020

openjck May 7, 2020

spasovski May 8, 2020

openjck May 7, 2020

openjck May 7, 2020

openjck May 7, 2020

spasovski May 8, 2020

hamilton May 8, 2020

openjck May 7, 2020

spasovski May 8, 2020

openjck May 7, 2020

openjck left a comment

hamilton left a comment

hamilton May 7, 2020

hamilton May 8, 2020

hamilton May 8, 2020

hamilton May 8, 2020

-    <p>This dashboard is under active development. Bugs and data accuracy issues may exist.</p>
+    <p>Thank you for testing the GLAM prototype!
+This tool is still in active development. UX bugs and data issues may exist.
+Help us make it suit your needs by directing questions or feedback to the #glam Slack channel.</p>

fixes #467 add data error alert notice #478

fixes #467 add data error alert notice #478

Conversation

spasovski commented May 7, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjck left a comment

Choose a reason for hiding this comment

hamilton left a 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

spasovski commented May 7, 2020 •

edited

Loading