#2865: Various notification improvements #3531

fregante · 2022-05-30T16:59:37Z

Part of Further notification improvements #2865

The remaining 3 points will have to wait and/or be followed by someone else (they depend on the library or need some React setup)

You can find the details of these changes in the review that follows.

fregante · 2022-05-30T17:47:35Z

src/utils/notify.tsx

 );

 function getMessageDisplayTime(message: string): number {
  const wpm = 100; // 180 is the average words read per minute, make it slower
-  return Math.min(
+  return Math.max(


Fix Notification timer (it was being ignored)

Fixed: All the notifications without a specified duration would last 2 seconds regardless of length

fregante · 2022-05-30T17:47:47Z

package.json

@@ -193,7 +193,6 @@
    "@types/json-stringify-safe": "^5.0.0",
    "@types/lodash": "^4.14.171",
    "@types/mustache": "^4.1.2",
-    "@types/notifyjs-browser": "0.0.0",


Drop outdated types (requires review)

I think that Notify.js’ types are somehow part of the public Alert brick’s config and the whole object was being passed as was to the notifier. However we kind of broke that months ago when we dropped the library. In practice I don’t think this was used for anything more than changing the className/type.

Now I'm making the last step of replacing the className setting with just type.

There's no migration path and I don't think it was ever necessary since error events should use error type, and so on…

Now I'm making the last step of replacing the className setting with just type.

It's probably fine, but I'd have to see if anyone (probably not) is using the following affordance in production: https://github.com/pixiebrix/pixiebrix-extension/pull/3531/files#r885082918

However we kind of broke that months ago when we dropped the library. In practice I don’t think this was used for anything more than changing the className/type.

Given your comment here, it sounds like if anyone was using it, we would have heard about it? If no one is, I will need to drop the support from menu item extension

What you're referring to is it that the old code destructured className? Or what exactly are you referring to?

{ message, config: { className } }: MessageConfig

That's correct. When I changed library months ago I mapped the className to the type property because that's how our code was using the library.

That's correct. When I changed library months ago I mapped the className to the type property because that's how our code was using the library.

Re-read some of our old documentation. With the old library you could pass props for controlling whether it was dismissible, etc. It wasn't very well documented and wasn't exposed in the Page Editor, though

fregante · 2022-05-30T17:48:10Z

src/utils/notify.tsx

-  <span>
-    {message}
+  <>
+    <span className={styles.message}>{message}</span>


Always place close button on the right + less margin-right

Before

After

fregante · 2022-05-30T17:48:30Z

src/utils/notify.tsx

+    id,
+    duration,
+    // Keep the notification centered on the document even when the sidebar is open
+    style: { marginLeft: `calc(var(${SIDEBAR_WIDTH_CSS_PROPERTY}, 0) * -1)` },


Keep notifications centered even with sidebar open

Before

This covers our own UI

After

Keeps the notification out of the sidebar, which could theoretically have its own notifications

Demo

Screen.Recording.mov

fregante · 2022-05-30T17:49:47Z

src/utils/notify.tsx

-  config: Partial<NotificationOptions>;
-}
-
-export function notifyResult(


Dropped, showNotification takes its place. extensionId wasn't used and now it just matches that function

twschiller · 2022-05-30T21:25:17Z

src/extensionPoints/menuItemExtension.ts

-          extension.id,
-          merge({}, onSuccess, DEFAULT_ACTION_RESULTS.success)
-        );
+        showNotification({ ...DEFAULT_ACTION_RESULTS.success, ...onSuccess });


@fregante changing the the API breaks this. This was an affordance for menu extension authors to customize the style/configuration of the notifications

I don't think anyone ever used the feature in production, but I'm going to have to double-check before merging in the PR

I forgot to comment on this, but I think this never worked. Didn't the order of the parameters here guarantee that the user's message and className properties would always be overridden by the DEFAULT_* object?

Didn't the order of the parameters here guarantee that the user's message and className properties would always be overridden by the DEFAULT_* object?

I think you're right. I probably had misread the lodash documentation and never tested it manually with the overrides

https://lodash.com/docs/4.17.15#merge Source objects are applied from left to right. Subsequent sources overwrite property assignments of previous sources.

twschiller

@fregante I need to double-check this before merging: #3531 (comment)

But had a question for you in that comment

…fications

codecov-commenter · 2022-05-31T01:43:42Z

Codecov Report

Merging #3531 (e0fdec8) into main (3d88bc3) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##             main    #3531   +/-   ##
=======================================
  Coverage   40.44%   40.44%           
=======================================
  Files         772      772           
  Lines       22474    22478    +4     
  Branches     4722     4725    +3     
=======================================
+ Hits         9089     9091    +2     
- Misses      12465    12467    +2     
  Partials      920      920

Impacted Files	Coverage Δ
src/blocks/renderers/customForm.tsx	`0.00% <0.00%> (ø)`
src/extensionPoints/menuItemExtension.ts	`9.46% <14.28%> (-0.09%)`	⬇️
src/contentScript/sidebar.tsx	`26.38% <25.00%> (+0.14%)`	⬆️
src/extensionPoints/quickBarExtension.tsx	`19.32% <33.33%> (ø)`
src/utils/notify.tsx	`71.42% <71.42%> (+2.41%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d88bc3...e0fdec8. Read the comment docs.

fregante added 5 commits May 31, 2022 00:40

Fix Notification timer (it was being ignored)

a00e28d

Always place close button on the right

9b5666c

Drop outdated Notify.js types

bb024d3

Re-center notifications when the sidebar is open

9be44f1

Clean

7933796

fregante commented May 30, 2022

View reviewed changes

fregante marked this pull request as ready for review May 30, 2022 17:58

fregante requested a review from twschiller May 30, 2022 17:58

twschiller reviewed May 30, 2022

View reviewed changes

twschiller assigned fregante and twschiller May 30, 2022

twschiller self-requested a review May 30, 2022 22:01

twschiller requested changes May 30, 2022

View reviewed changes

twschiller added this to the 1.6.4 milestone May 31, 2022

twschiller added 2 commits May 30, 2022 21:31

Merge remote-tracking branch 'origin/main' into F/feature/better-noti…

d58a629

…fications

Restrict notification properties

e0fdec8

twschiller approved these changes May 31, 2022

View reviewed changes

twschiller enabled auto-merge (squash) May 31, 2022 01:39

twschiller merged commit d2548be into main May 31, 2022

twschiller deleted the F/feature/better-notifications branch May 31, 2022 01:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2865: Various notification improvements #3531

#2865: Various notification improvements #3531

fregante commented May 30, 2022 •

edited

Loading

fregante May 30, 2022

fregante May 30, 2022 •

edited

Loading

twschiller May 30, 2022 •

edited

Loading

fregante May 31, 2022

twschiller May 31, 2022

fregante May 30, 2022

fregante May 30, 2022

fregante May 30, 2022

twschiller May 30, 2022

fregante May 31, 2022

twschiller May 31, 2022

twschiller left a comment

codecov-commenter commented May 31, 2022

#2865: Various notification improvements #3531

#2865: Various notification improvements #3531

Conversation

fregante commented May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Fix Notification timer (it was being ignored)

fregante May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Drop outdated types (requires review)

twschiller May 30, 2022 • 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

Always place close button on the right + less margin-right

Before

After

Choose a reason for hiding this comment

Keep notifications centered even with sidebar open

Before

After

Demo

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

twschiller left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 31, 2022

Codecov Report

fregante commented May 30, 2022 •

edited

Loading

fregante May 30, 2022 •

edited

Loading

twschiller May 30, 2022 •

edited

Loading

Always place close button on the right + less `margin-right`