Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding snackbar notices support to the new widgets page #16020

Merged
merged 2 commits into from Jun 17, 2019

Conversation

@youknowriad
Copy link
Contributor

commented Jun 6, 2019

This PR adds a "save success" snackbar notice to the new widgets page. It's also the first PR adding support for the notices to the widgets screen which raises a bunch of interesting questions :)

Testing instructions

  • Click the "update" button in the widgets screen
  • A success notice should show up

@youknowriad youknowriad self-assigned this Jun 6, 2019

import { SnackbarList } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';

function Notices() {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 6, 2019

Author Contributor

This made me wonder if we want to move the EditorNotices component from the editor package to the notices package as we'd have to rewrite such component for each consumer.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

Yes, I guess it would be good to avoid this code repetition and just expose the component as part of the notices package.

__( 'Block areas saved succesfully.' ),
{
id: WIDGET_AREAS_SAVE_NOTICE_ID,
type: 'snackbar',

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 6, 2019

Author Contributor

I wondered whether the "context" argument should be used here to differentiate between the "post editor" notices and the "widget screen" notices. Imagine a world where the WP-Admin is a full SPA, do we want to use this context to differentiate between these notices?

Also depending on the answer, this raises the question of whether we should set a default context to "post editor" for existing plugins creating notices in the editor.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

Hi @youknowriad, you mean the context for translation right? If yes even if we a are in a full SPA I don't think we need context to differentiate between screens. WordPress did not used different contexts if two strings were on different screens so I think we can follow the same logic.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 14, 2019

Author Contributor

no, I meant the "context" property of the notices (we never used it but the API supports it). cc @aduth as we discussed some of this recently.

@jorgefilipecosta jorgefilipecosta added this to Widget screen UI issues in Block Editor For Widgets Jun 12, 2019

@jorgefilipecosta
Copy link
Member

left a comment

Things worked great in my tests, thank you for this important UX improvement 👍 I guess the would be good to a avoid the need to implement the notices component as part of this package (packages/edit-widgets/src/components/notices/index.js), but I'm ok with that refactored being performed in a different PR.

import { SnackbarList } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';

function Notices() {

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

Yes, I guess it would be good to avoid this code repetition and just expose the component as part of the notices package.

args: [
'Block areas saved succesfully.',
{
id: 'WIDGET_AREAS_SAVE_NOTICE_ID',

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jun 13, 2019

Member

Nit pick: Maybe the test can import WIDGET_AREAS_SAVE_NOTICE_ID as a constant from the actions file.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jun 17, 2019

Author Contributor

It's not exported at the moment. I mean I could export it but for a test, maybe it's not that important.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I tested, and from a design perspective, it looked great. :shipit: I may have missed it, but wanted to verify there was agreement on the time the snackbar remained visible before dismissing itself. I love these little notifications animating into view!

@youknowriad youknowriad merged commit 1d653d1 into master Jun 17, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the add/widgets-notices branch Jun 17, 2019

@jorgefilipecosta jorgefilipecosta moved this from Widget screen UI issues to Core/Extensibility/API tasks in Block Editor For Widgets Jun 17, 2019

@jorgefilipecosta jorgefilipecosta moved this from Core/Extensibility/API tasks to Done in Block Editor For Widgets Jun 17, 2019

@youknowriad youknowriad added this to the Gutenberg 6.0 milestone Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.