Notification scripts update #92
Conversation
There was a problem hiding this comment.
@erikyo thanks for all your hard work here! I've added a few comments but in general this has massively pushed things forward.
As a general note I'd like to do some work on modularisation and comments in particular, but I think it would make more sense to get this merged and then we can chip away at any updates in smaller PRs (since this is pretty huge at this point).
@bacoords @jonathanbossenger what are your thoughts? Should we go ahead with a merge so we can work against develop again?
| import { delay } from '../utils'; | ||
|
|
||
| // Notice class method. | ||
| export default class Notice extends Component { |
There was a problem hiding this comment.
Does this have to be a class? Or could it just be const or function? It's not got any class-specific logic or anything.
There was a problem hiding this comment.
For certain things I have always preferred to create a class whereas I use functional components more often for component elements, this one has also a method (onDismiss) and I guess more may be added in the future..
In this way the component is also more prone to being extended (not that it would be impossible otherwise)
| this.props.additionalClassName, | ||
| ].filter( Boolean ); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
This is good, but it is a bit of a massive thing 😅
Maybe not now, but it would be good to split it up slightly, so something like this for each of the individual conditional components.
const dashboardActions = () => <something>Some buttons</something>
return (
<div>
{ (location === 'dashboard') && dashboardActions }
</div>
)There was a problem hiding this comment.
Following this idea I also created a component for the actions (ok+dismiss button), but I was wondering if it would be a good idea to do the same for the title/text. What do you think?
…g message in console
Better code splitting
…rmat replaces the hardcoded notifications
…ation (fadeOff animation included)
Enqueue the notify script after wp-components and wp-i18n has been loaded
updates dependencies rearrange code
…imbly style the border since it would now be a bit complex to do added more jsdocs
…ss#108) not 100% sure if it is wai compliant we need to create the shortcut for action (space?) / delete (del ?) etc. https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#kbd_roving_tabindex
…ive, but need an empty alt attribute to prevent screen readers from attempting to interpret them. (addresses WordPress#107)
focus / active single hub notification
Co-authored-by: Joe Bailey-Roberts <[email protected]>
Co-authored-by: Joe Bailey-Roberts <[email protected]>
this is somehow related to WordPress#110 because it makes the drawer open when selected with tab
fix for an action dispatch that was called incorrectly
|
since 989c918 checks are failing since i've removed the rules exclusions in the phpcs configuration file. I think if we want use the WP-standards we can't pick and choose the ones we like or don't like and that since it's only loaded the ruleset of core and doc we should follow it. |
@erikyo for now can you just revert the phpcs config change? While I agree that it's a change worth making, I'd prefer to make it in its own followup PR. It's a little outside the scope of JS scripts update, and ideally something to handle separately rather than delaying merging this into develop. |
NoticeAction component updated with the standard WordPress css classes
There was a problem hiding this comment.
💥 Awesome stuff!
Re: the changes to the json file, I think this is fine for now, although I'd like to get the ball rolling again on the schema discussion and finalise this before we go much further.
There's a lot in here and probably a few things that could be tweaked, but for now I'd be in favour of merging this and iterating on things in smaller PRs going forward.
Since the previous version has all hub notifications hardcoded and the hub could not be tested, I created a react component and a redux store controller to handle notifications multiple destinations. The component has a context (formerly called location), which subscribes to notifications from its context and receives updates from it.
Then
notifyControllerwas rebuilt with all the previous functionality (add, remove, clearAll) and, in addition, a function was added to retrieve the api (it currently reads a false response from a json file).Actually we have two main components:
NotificationandNotificationArea, the latter depending on context displays the array ofNotificationwith diffent styles and options (for example at the moment the hub is set to display two lists of notifications, the current week's and previous week notices. This is not yet customizable but it would be nice if we can provide some filters (by type, by plugin etc as mentioned in PR #84)We have a rest-api endpoint (thanks @Sephsekla!) but we need to decide how the component fetches data from the database, what arguments it passes to it, and the strategy for updating the data in the reducer.
a brief story of what is changed with this PR:
@wordpress/componentsin order to comply to wordpress design standards@wordpress/datato dispatch notifications to all components that request themaddNotifyhas been added to window.wp.notify (the js/api was located there)https://github.com/erikyo/wp-notify/blob/443873fc9e93cdbe83e92df64ff1708de85b4b4f/src/scripts/wp-notify.js#L50