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

Adds an is-busy state to the save button of the widgets page #16019

Merged
merged 1 commit into from
Jun 7, 2019

Conversation

youknowriad
Copy link
Contributor

This PR just updates the save button of the widgets screen to apply the is-busy and aria-disabled states when the widgets are being saved.

I also extracted the button to its own component. Notice that the design of the busy state is not great at the moment but this is not specific to the widgets screen and is tracked separately.

I also wanted to add "snackbar notifications" to the save success action but decided to that separately?

@youknowriad youknowriad self-assigned this Jun 6, 2019
@youknowriad youknowriad added the [Feature] Widgets Screen The block-based screen that replaced widgets.php. label Jun 6, 2019
const [ isSaving, setIsSaving ] = useState( false );
const { saveWidgetAreas } = useDispatch( 'core/edit-widgets' );
const onClick = useCallback( async () => {
setIsSaving( true );
Copy link
Contributor Author

@youknowriad youknowriad Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me wonder if we'd need a isPerforming selector for actions similar to the isResolving one we have for resolvers. cc @aduth

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea but wonder why not also name it isResolving since, to me, having an entity saved is a 'resolution'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me wonder if we'd need a isPerforming selector for actions similar to the isResolving one we have for resolvers. cc @aduth

Built-in? Or as a pattern?

Generally I'd like if these could actually be resolvers, so we're not setting any state, the state is reflected by the selection of the existing isResolving on the widgets save resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking built-in similarily to resolvers but for actions. Technically speaking resolvers and actions are the exact same thing right now (in terms of code), the only different is that one get triggered when you call a selector, the other when you call an action :)

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks perfect, and when I test this locally with network throttling on I see that it's working.

One bug I noticed that exists also in the post editor is that the button text is white which is hard to read:

Screen Shot 2019-06-07 at 15 45 22

edit: Ah, yep, you mentioned this in the description! 🙂

@youknowriad youknowriad merged commit f410c46 into master Jun 7, 2019
@youknowriad youknowriad deleted the add/is-busy-state-widgets-save-button branch June 7, 2019 08:09
@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019
@aduth
Copy link
Member

aduth commented Jun 7, 2019

busy state is not great at the moment but this is not specific to the widgets screen and is tracked separately.

Specifically: #15029

const { saveWidgetAreas } = useDispatch( 'core/edit-widgets' );
const onClick = useCallback( async () => {
setIsSaving( true );
await saveWidgetAreas();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not realize this is a thing we could be doing now... and I'm not sure how I feel about it 😅

const [ isSaving, setIsSaving ] = useState( false );
const { saveWidgetAreas } = useDispatch( 'core/edit-widgets' );
const onClick = useCallback( async () => {
setIsSaving( true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me wonder if we'd need a isPerforming selector for actions similar to the isResolving one we have for resolvers. cc @aduth

Built-in? Or as a pattern?

Generally I'd like if these could actually be resolvers, so we're not setting any state, the state is reflected by the selection of the existing isResolving on the widgets save resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants