Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Expand WP notices with header toggle #101

Merged
merged 5 commits into from
Jun 4, 2018
Merged

Conversation

justinshreve
Copy link
Collaborator

See #77.

Per p6riRB-3bd-p2 #comment-3810, this is an attempt at putting WP notices, hidden by default, behind a toggle that will display when there are notices present.

See this for a preview: https://cloudup.com/cUScXAMJH-R

It does some DOM manipulation that doesn't feel great, but if we hope to show these in a nice way to the user, that seems necessary no matter what based on how WP notices work (just HTML dumped into wp-admin -- no array or structure). Considering this a try branch for now incase someone has another idea.

To Test:

Copy link
Member

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Works as described 👍 Given the WP notice system I think this is a good-enough solution, maybe we'll get something better from Gutenberg eventually.

I also tested with a dismissable notice that adds an AJAX request to persist the dismiss state, and that worked too 👍

this.maybeAddDismissEvents();
}

this.setState( Object.assign( {}, { noticesOpen: ! noticesOpen } ) );
Copy link
Member

Choose a reason for hiding this comment

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

Why the Object.assign in setState?

@justinshreve
Copy link
Collaborator Author

@jameskoster @josemarques How is this? I know the button/toggle might change so we can consider that temporary and subject to change, but does this work for actually displaying them?

@timmyc timmyc added this to Needs Review in Isotope May 31, 2018
@jameskoster
Copy link
Member

It works! The original plan was to move the entire UI down to reveal the notices like this;

notice

That helps distinguish these notices as a separate thing.

@justinshreve
Copy link
Collaborator Author

@jameskoster I tried that initially (it's the default location for them) and thought maybe it would be better to not have the header bar get moved. There is a few lines of JS to move them around so that part is easy to change. So, if you think that would be better to have them separate, I can leave them above the bar.

Preview:

screen shot 2018-06-01 at 8 32 15 am

@ryelle
Copy link
Member

ryelle commented Jun 1, 2018

If we do that, we'll need to do some focus management to make sure screen reader/keyboard users are navigated back to the notices (kind of like how dropdowns work). Currently this PR works "by default" because they come up next in the tab order.

@justinshreve
Copy link
Collaborator Author

Thanks for the note there. I'll make sure to handle that if we decide to put the notices there.

Just rebased this against master + changed CSS class names.

@jameskoster
Copy link
Member

Good point. Spoke with @josemarques about this and we decided your original approach is probably best overall.

notices

@justinshreve justinshreve merged commit c725bef into master Jun 4, 2018
Isotope automation moved this from Needs Review to Done Jun 4, 2018
@justinshreve justinshreve deleted the update/wpnotices branch June 4, 2018 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants