Global Notices: support action link, timeout and animation. #4821

Closed
wants to merge 19 commits into
from

Projects

None yet

9 participants

@taggon
Member
taggon commented Apr 18, 2016 edited

This PR enhances Global Notices and attempts to tackle the tasks in #2958. You can go through how I solved each items from #2958 one by one. (The sentence starting with a checkbox is a task @rickybanister listed.)

Global Notices

Stack-up

  • allow more than one notice to stack up, rather than new notices replacing older ones—they should create a nice list

Now the global notices will be stacked up vertically by default. If you want to replace an old notice with new one, read “Replacing a notice” section.

New Options

  • style notice-action to allow for action links inside global notices (they should allow for gridicons as well)
  • allow notices to have either a timeout (to be fully ephemeral, like for 'Saved!' notices)

The notice action creators accept the three extra options:

  • button - Text label to display on action button.
  • duration - Duration in milliseconds to display the notice before dismissing.
  • icon - Any Gridicon's name.

If you want to show a global notice that contains an action button, just set the button option like this:

import { successNotice } from 'state/notices/actions';

/* ... */

successNotice( 'Hello', {
    button: 'Action'
} );

The notice will show an action button whose label says Action. Unfortunately, the redux documentation says:

By convention, the top-level state is an object or some other key-value collection like a Map, but technically it can be any type. Still, you should do your best to keep the state serializable. Don’t put anything inside it that you can’t easily turn into JSON.

So I had to make function-type notice options like old onClick event handler obsolete. Consider using the notice mixin if you need to listen to click event on action buttons.

The durationis the same with the old notices option.

New Animations

  • More fun animation—take a cue from the Reader 'new posts' indicator and have the notices slide in from stage left, and fly out the same way

Now Global Notices supports animation prop that accepts an object indicates enter and leave animations. Available animation types are fade, fade-left, fade-right, fade-up, fade-down and zoom. The following code applys fade-left and fade-right effects on the notice appearing and disappearing respectively.

<GlobalNotices animation={{enter: 'fade-left', leave: 'fade-right'}}> />

Notice Replacement

  • allow notices to be replaced by another notice (like 'Activating Plugin' => 'Plugin Activated')—they would have to be sub-grouped or something

A new notice always replace the old one that has the same ID. Suppose you want to change a text in a global notice whose ID is notice-1 and the new text is "Plugin Activated". You can write the code like this:

import { infoNotice } from 'state/notices/actions';

infoNotice( 'Plugin Activated', { id: 'notice-1' } );

The Reader's New Post Indicator

  • Convert the Reader 'new posts' indicator to be a global notice so it doesn't collide with them.

Now the indicator is a global notice. Because currently we only support success, error, info and warning, is-update status and updateNotice() action creators are newly added to display the indicator.

updated

## Notice Mixin

Notice mixin helps you with creating and managing global notices. I'd recommend you apply the mixin to components that create a global notice. For more detail, read the documentation.

taggon added some commits Apr 17, 2016
@taggon taggon Global Notices: some nice to haves (#2958)
- Allow notices to include an action link.
- Allow notices to have a timeout.
- Add 1px of top margin to a notice.
- Improve a bit of performance.
- New notice replaces the old one has the same ID.
- More than one notice will stack up.
- The Reader uses Global Notices for 'new posts' indicator.
- Update the design example to use redux action creators instead of old notice functions.
57ad57b
@taggon taggon Global Notices: add animation effects 03065d1
@taggon taggon Set fade as default animations. bc3ff14
@taggon taggon Notices: change ID property name to `noticeId` from `id`. 425292f
@taggon taggon Notices: add a test case for old notice with the same ID d58bed2
@taggon taggon added the Components label Apr 18, 2016
@rickybanister
Member

Hey, this is looking pretty good.

I think we should probably use the orange color still for the Reader notice instead of the blue. That matches the notifications dot and seemed to work well before.

screen shot 2016-04-18 at 4 15 09 pm

@taggon
Member
taggon commented Apr 19, 2016

@rickybanister That makes sense.
I restored the orange color and, plus, I added className option to a notice. It will allow you to customize notices.

updated

@mtias mtias and 1 other commented on an outdated diff Apr 19, 2016
client/reader/following-stream/_style.scss
@@ -373,3 +373,7 @@
font-size: 12px;
line-height: 1;
}
+
+.reader-stream-notice.notice.is-info {
@mtias
mtias Apr 19, 2016 Member

I think we should perhaps create an update status or similar for notices that is orange instead.

@taggon
taggon Apr 19, 2016 Member

It's a good point. But I'm wondering if we have enough potential use cases for update status.

@mtias mtias and 1 other commented on an outdated diff Apr 19, 2016
client/reader/following-stream/index.jsx
+ this.removeNotice();
+ }
+ }
+ },
+
+ updateNotice: function( count ) {
+ let countString = count >= 40 ? '40+' : ( '' + count );
+
+ this.context.store.dispatch(
+ infoNotice(
+ this.translate( '%s new post', '%s new posts', { args: [ countString ], count: count } ),
+ {
+ id: READER_STREAM_NOTICE_ID,
+ icon: 'arrow-up',
+ showDismiss: false,
+ className: READER_STREAM_NOTICE_ID,
@mtias
mtias Apr 19, 2016 Member

Classes should follow the namespace__element convention.

@taggon
taggon Apr 19, 2016 Member

Updated. :)

@mtias
Member
mtias commented Apr 19, 2016

This is a great approach that cleans up custom reader code and improves global notices.

Calling @artpi as he has spent time thinking and working on these for a deeper look.

@mtias mtias added the Framework label Apr 19, 2016
@rickybanister
Member

I agree with @mtias here, maybe we could add an 'is-update' prop for orange notices.

The color would be $orange-jazzy

@taggon
Member
taggon commented Apr 21, 2016

@mtias @rickybanister Thanks for your feedbacks. I added update status and updateNotice action creator.

@taggon
Member
taggon commented Apr 25, 2016

@rickybanister @mtias @artpi Any other suggestions or comments?

@rickybanister
Member

Looks good to me.

@mtias?

@artpi artpi and 1 other commented on an outdated diff May 2, 2016
client/state/notices/actions.js
const notice = {
- noticeId: options.id || uniqueId(),
- duration: options.duration,
+ noticeId,
+ icon: options.icon || null,
+ duration: parseInt( options.duration ) || null,
@artpi
artpi May 2, 2016 Contributor

I don't know how babel handles it, but for safety reasons its always good to include radix while using parseint: http://stackoverflow.com/questions/6611824/why-do-we-need-to-use-radix

@taggon
taggon May 5, 2016 Member

Okay, I'll add it. :)

@artpi
Contributor
artpi commented May 2, 2016 edited

수고했어요 !

Nice work @taggon !

I don't think it's a blocker for this PR, but maybe I'll explain whats going on with the notices reduxification effort (which has halted a bit) and why its not done yet:

Actions

In some places (as I recall, around jetpack and store) action or dismiss triggers a function. That is a problem in new reduxified notices, because we don't want functions in notices redux tree. We were discussing with @mtias that we can pass a redux action to trigger once "NoticeAction" is clicked to cover those cases, but that would require listening for those actions also.
Now that @dmsnell has merged #4699 with new analytics middleware, all cases where custom function is used only for analytics can be approached this way.

Components

In some cases, components are injected via translate. We also don't want them in the redux tree

Many global notices use cases that use custom functions and components can be converted to just use notice component and sometimes it would benefit UX if notice appeared in a proper place.

Action creators

Third hurdle to fully convert all notices to new redux tree is that component issuing a notice has to be connected to be wrapped in a provider. Sometimes that has sideeffects and breaks stuff.

Once these 3 problems are dealt with, we will be able to reduce old global notices, clean up a lot of code (like client/notices tree) and simplify stuff.
Just an explanation if you are looking for something to do :)

I intend to go back to these efforts some day, but I can't find time to handle it right now :/

@blowery blowery commented on the diff May 3, 2016
client/reader/following-stream/index.jsx
@@ -97,6 +106,38 @@ module.exports = React.createClass( {
this.showSelectedPost( { replaceHistory: true } );
}
}
+
@blowery
blowery May 3, 2016 Contributor

should we also check the updateCount state in componentDidMount, in case the stream renders with updates available? we don't currently automatically accept new posts when mounting the stream.

@taggon
taggon May 6, 2016 Member

@blowery Though the problem seems to be outside the scope of this PR, I prefer auto updating too. How about creating a new issue and having discussions there?

@nb nb commented on the diff May 5, 2016
client/state/notices/README.md
## Options
The first argument is the text to be displayed on the notice. The second argument is an optional object and accepts some properties:
*All these parameters are optional*
-* `id` (default generated `uniqueId()` ) ID for your notice
+* `id` (default generated `uniqueId()` ) ID for your notice. If a notice with the same ID already exists then the new one will replace it.
@nb
nb May 5, 2016 Member

Can we add an example about notices replacing each other? Maybe not with highest priority and I am ok leaving for another PR.

@taggon
taggon May 5, 2016 Member

No problem. I'll add it to the example page.

@nb nb commented on the diff May 5, 2016
client/state/notices/test/reducer.js
@@ -48,6 +48,20 @@ describe( 'reducer', () => {
] );
} );
+ it( 'should properly replace old notice with new notice that has the same ID', () => {
@nb
nb May 5, 2016 Member

Thanks for adding a test for this, it made the functionality clearer for me.

@nb nb and 1 other commented on an outdated diff May 5, 2016
client/state/notices/README.md
* `duration` (default null - notices stay forever) Duration in milliseconds to display the notice before dismissing.
* `showDismiss` (default true) To indicate if dismiss button should be rendered within the overlay.
* `isPersistent` (default false - notices disappear when navigating route) - should notice be persistent between route changes?
* `displayOnNextPage` (default false) - should notice appear on next route change?
+* `className` (default null) - Additional class names to be applied to the notice.
@nb
nb May 5, 2016 Member

Do we need to expose as part of the public interface? I don’t see it used anywhere.

@taggon
taggon May 5, 2016 Member

No, we don't. I'll remove the option because it is not as useful as I intended at first.
The reason why I added the option was to show update notiice by extending is-info notice.
Since it can be used to change color or icon of notice I kept the option to provide custom look even after adding is-update.

@taggon
Member
taggon commented May 5, 2016

@artpi
Wspaniały! 👍
That’s really what I should know.
Let me make sure that I understand you correctly and ask you some questions.

because we don't want functions in notices redux tree.
In some cases, components are injected via translate. We also don't want them in the redux tree

Before putting into action may I know the reason why functions and components shouldn’t be in the redux tree? It can help me find a way to avoid them.

Many global notices use cases that use custom functions and components can
be converted to just use notice component and sometimes it would benefit UX if notice appeared in a proper place.

I fully agree with you on the point that “it would benefit UX if notice appeared in a proper place”. But I thought Global Notices can be the proper place because notifications in one place would be better than scattered notifications.
And in fact I was tackling the #2958 created by @rickybanister . So I want to hear from @rickybanister too. 😄

Sometimes that has sideeffects and breaks stuff.

Could you elaborate on what problem we will have? It would be a great help to me to find the solution.

@artpi
Contributor
artpi commented May 6, 2016

Before putting into action may I know the reason why functions and components shouldn’t be in the redux tree?

Our intention is to keep redux tree as clean and pure as possible to have it serializable. If in far-far future redux would be pure, we could:

  • Serialize it and reproduce any bug just by dropping state somewhere
  • Sync state via socket.io
  • Serialize state for offline

For now, notices are not serialized and persisted, but there is no reason they shouldn't be able to.
I highly recommend https://github.com/Automattic/wp-calypso/blob/master/docs/our-approach-to-data.md
When it comes to the redux tree, you may want to catch @mtias @gwwar and @aduth .

notifications in one place would be better than scattered notifications.

Imagine you have a very long settings form, like https://wordpress.com/settings/general/ .
You make a mistake in 1 field deep down in form and then you submit and you get 3 global notices each about a different field.
Now you have to look for these fields by the message and field name. Plus you don't know how these labels and your error message will be translated so it can even have different name in the first attempt.

Alternatively, if you have a notice appearing right by the field you made a mistake in, you can just look at the site to identify the fields you want to correct. You don't even need to read the errors.

You can either group information by:

  • type of it "errors here, forms there"
  • context of it "field and error for discussion settings here, field and error for email there"

And I think second approach is more natural for the user.

I think that global notices should be used for general events and things that are not on the page. Like API errors and information that form was not saved. But they are bad for providing detailed error messages and suggesting fixes.
@mtias

Could you elaborate on what problem we will have?

Regarding redux Provider:

In some cases while I was wrapping render trees in redux provider, some controllers were taking output of React.render and operate directly on it, for example change props and some other hacks.

This will not work if you wrap stuff in provider, because you add an extra layer of abstraction and you need to do things properly.

Also, a lot of test break when you wrap component in connect, but that can be usually fixed by either exporting pure component as well as wrapped, or refering to .WrappedComponent on wrapped one.

@rickybanister
Member

I fully agree with you on the point that “it would benefit UX if notice appeared in a proper place”. But I thought Global Notices can be the proper place because notifications in one place would be better than scattered notifications.
And in fact I was tackling the #2958 created by @rickybanister . So I want to hear from @rickybanister too. 😄

Sorry, I think I missed the question. On which feature/bit would you like an opinion?

@taggon
Member
taggon commented May 7, 2016

@rickybanister
It is about an item in #2958

You said:

Convert the Reader 'new posts' indicator to be a global notice so it doesn't collide with them.

And @artpi said:

I think that global notices should be used for general events and things that are not on the page.

It seems there is a disagreement on the usage of global notice.

In my opinion, a twitter-like update notice(refer to the screenshot below) for the Reader 'new posts' indicator can meet both requirements: not colliding with global notice, and using global notices for more general events.

twitter_notification

So, what do you think about @artpi’s opinion and mine?

@taggon
Member
taggon commented May 7, 2016

@artpi

Our intention is to keep redux tree as clean and pure as possible to have it serializable.

Understood. I got an idea about how to fulfill the intention but it will take some time to make it work. I think I can show you the result by tomorrow.

I think that global notices should be used for general events and things that are not on the page. Like API errors and information that form was not saved. But they are bad for providing detailed error messages and suggesting fixes.

I got your point and I agree with you.

Regarding redux Provider:

If Provider or connected components cause troubles, we can use Global Notices with context. Components such as TinyMCE and Popover are using this approach.

What do you think of this approach? If you agree, I will update Global Notices to use context.

@rickybanister
Member

@taggon Ah, I understand.

So since global notices were born out of the design for the Reader new post indicator we always intended to replace it with one.

They are also used 'on page' in many places as an ephemeral 'Saved' indicator and other things. I wouldn't say they are for 'general events or things not on the page'. They are for things outside of the immediate context/viewport—like if there's an error with something far up on the current page, if an import from another page failed, if there are new items to read off screen.

I'm only referring to the UI however, they indicate to a user that something is happening outside of their current view. Code-wise that may not be 100% accurate, but I think it makes sense to use this component for the Reader.

@taggon taggon Global Notices: add notice mixin.
The mixin provides some helper functions that create and remove notices.
With this mixin, we don't need `onClick` option anymore. So I removed it.
5a72c8a
@taggon
Member
taggon commented May 11, 2016 edited

@artpi It took more than I thought.
I added notice mixin to remove functions from redux tree. That means the onClick option is removed now. See the documentation for more detail and the Reader new posts indicator[1][2] would be a good example.
I would appreciate your feedback.

@taggon
Member
taggon commented May 11, 2016

@artpi @ricky Thanks!

I missed cases like "Saved" indicators. So, I looked up more and found out that Calypso’s page-level notices exist in both global-notice and not-a-global-notice type. For instance, while the "Saved" indicator is a global notice in My Profile page, it is a not-a-global-notice in the editor form. Each approach clearly has its own advantages and disadvantages.

  • My Profile
    2016-05-12 1 27 07
  • editor form
    2016-05-12 1 27 50

To change UI we have to spend more time. Plus, it may harm UX. Therefore, when it is hard to tell which one is way better than the other, maintaining the status quo can be more reasonable. Fortunately, the former Reader new post indicator looked like a global notice. Converting it to a global notice hardly change the user experience.

@artpi
How about make a new issue for discussing where to use Global Notices? Because, though it certainly is helpful for me to see what’s going on with the notices reduxification effort, it may be getting too deep for this issue.

@nb nb commented on an outdated diff May 12, 2016
client/state/notices/reducer.js
@@ -47,6 +48,24 @@ export function items( state = [], action ) {
return state;
}
+export function clicked( state = null, action ) {
@nb
nb May 12, 2016 Member

Just to make sure I understand, this part of the state is holding the ID of the latest clicked notice? We can maybe think of a more descriptive name, since it took me a while to figure this out.

@nb nb and 1 other commented on an outdated diff May 12, 2016
client/lib/mixins/notice/index.jsx
@@ -0,0 +1,79 @@
+import React from 'react';
+import { successNotice, errorNotice, infoNotice, warningNotice, updateNotice, removeNotice } from 'state/notices/actions';
+import get from 'lodash/get';
+import uniqueId from 'lodash/uniqueId';
+import escapeRegexp from 'lodash/escapeRegexp';
+
+function forceInclideIdPrefix( idPrefix, noticeActionCreator ) {
@nb
nb May 12, 2016 Member

Was this meant to be force**Include**Prefix?

@taggon
taggon May 12, 2016 Member

It was a typo. Fixed. :)

@nb
Member
nb commented May 12, 2016

@taggon thanks, this is looking really good. The community is going away from mixins, though. React will drop support soon. Can we think of another solution? Few ideas: a wrapper component or just explicitly calling some helper functions in componentDidMount and componentWillUnmount.

@taggon
Member
taggon commented May 12, 2016

@nb Okay, for now a wrapper component sounds great. But let me think about it a bit more.

@taggon taggon Global Notices: added getHelpers function.
It allows to use the notice mixin module without mixins.
0564a07
@taggon
Member
taggon commented May 17, 2016

@nb now we can use the noticeMixin module without mixins.

At first I was considering using a wrapper component. But @artpi said that wrapper components fail a lot of test. So I found the new approach that doesn't use any wrapper component.

Since the mixin is still available, we can use it until Calypso completely drops all mixins.

See the documentation for more detail.

@apeatling
Member

@nb What are your thoughts on this one? It's a nice update, can we get it in?

@nb
Member
nb commented Jun 23, 2016

@apeatling sorry for the delay, it’s pretty much good to go, I can look at merging it early next week.

@taggon, could you rebase and clean up the branch history (remove fix commits, etc.)?

@lancewillett
Member

Closing as this is a several months old and possibly abandoned. Feel free to re-open if still valid and in-progress. Other work on notices has happened in the meantime.

@lancewillett lancewillett deleted the update/2958-improvement-on-global-notice branch Oct 4, 2016
@rickybanister
Member

It may be more valuable to extract out the interesting and applicable bits into smaller separate PRs—for instance the duration. I know @beaulebens and I have been hoping to make 'Saved!' notices more ephemeral for quite a while.

Maybe I can bribe someone to split it up?

@johnHackworth @tyxla @ebinnion ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment