New explorer using the admin API, and React tooling #3012
Conversation
|
@kaedroho especially, I have rebased |
| this.props.init(); | ||
|
|
||
| document.body.style.overflow = 'hidden'; | ||
| document.body.classList.add('explorer-open'); |
There was a problem hiding this comment.
Should the overflow be put in the CSS to keep it in one place?
There was a problem hiding this comment.
👍 Moved the overflow:hidden; to the properties of .explorer-open
| background: rgba(0,0,0,0.5); | ||
| } | ||
|
|
||
| [aria-role='presentation'] { |
There was a problem hiding this comment.
If this is only for the Icon titles - which look like they are there for accessibility display: none; can cause issues for some screen readers. For this case I tend to reach for the HTML5 boilerplate .visuallyhidden implementation which as been well battle tested.
There was a problem hiding this comment.
👍 I changed this to simply using the visuallyhidden class that already exists within the codebase.
|
Awesome man! |
Yep, this is exactly what I'm after. Thanks! |
|
Progress update: I've been refactoring the code and adding more tests. The JS linting is now successfully passing, although just barely. I would love to get a green CI build but I'm having trouble with Drone's caching (http://readme.drone.io/usage/caching/) which cannot easily be reset nor changed by pull requests it seems. I think that in order for this to look like a real upgrade once it is released it would be valuable to have more powerful features as part of the explorer – like some filtering, or pagination, or quick actions within the menu items. However right now I'm still focusing on the code quality because I'd like this to be an example of how other future UI components should be written. I think there is room to do both if other people want to actively work on this :) |
|
Just running this locally, noticed a couple of issues:
|
|
So long as this still respects changes made via the |
|
@ababic 👍 I'm keeping track of other Wagtail developments to make sure this doesn't step on too many toes. There's a (living) list of things I think are related at the end of the PR message – I have yet to figure out which ones really are related, and how they will be addressed 😁. |
thibaudcolas
force-pushed
the
feature/react-explorer-wip
branch
from
c6bb1af
to
03e1a00
Compare
thibaudcolas
force-pushed
the
feature/react-explorer-wip
branch
from
03e1a00
to
0303b7f
Compare
|
Test findings
|
thibaudcolas
changed the title
|
Did some additional testing after the latest changes. Currently it shows the following: |
|
I think the explorer PR is ready to be merged, apart from 3 things where I need help:
If you want to try the new explorer, https://wagtaildemo-springload.herokuapp.com/admin/ is available but I've also made a hacky self-XSS script to try it on any site that has the admin API v2. See https://gist.github.com/thibaudcolas/9edd4614a716ac7fbc9633f10908f316 You can paste this one-liner into the console and customise the two parameters to get the explorer (all scripts and styles) injected into the page, and talking to the site's admin API. TODO
Cross-browser tests
Last issues
Tasks backlog post-merge
WIP changelog
|
|
Thanks @robmoorman to put your hand up to review this 😉 |
thibaudcolas
force-pushed
the
feature/react-explorer-wip
branch
2 times, most recently
from
89f25c5
to
00bd19c
Compare
|
Removing the Edit: works on Windows 8 / Cygwin, works on OSX Yosemite, works on Ubuntu / Vagrant. |
* use focus-trap-react * switch clickable span to button * let focus trap handle outside click
Pin eslint-config-wagtail version to prevent conflicts with minor rule changesets
thibaudcolas
force-pushed
the
feature/react-explorer-wip
branch
from
47da13b
to
eb6c971
Compare
|
This could be a problem with packages that don't use API Integration( |
|
@SalahAdDin I don't get what you're taking about – what could be a problem? And is it https://github.com/infoportugal/wagtail-embedvideos you're referring to? |
|
I think they need implements API endpoint for this package works with this new admin, isn't it? |
|
No, this sounds unrelated. This explorer is just for pages, and we're not removing any of the other ways for packages to integrate with Wagtail. Please create an issue directly on the https://github.com/infoportugal/wagtail-embedvideos repository if you think there is an issue there. |
Tasks TODO before merge
torchbox:admin-apiwagtaildemoinstance so people can easily test it. – https://wagtaildemo-springload.herokuapp.com/admin/Features
Support for sites without homepages?– edit: not sure that's actually a thing? – marking as done.Page filters (with or without children) (currently an A/B toggle in the header for testing purposes)– Removed the filterAdds 'exclude_from_explorer' attribute to ModelAdmin class #2878 "Adds 'exclude_from_explorer' attribute to ModelAdmin class" make sure this works the same with the API.– Check. Usesconstruct_explorer_page_queryset, which is about the listing pages not the menu.Fix: Use specific page model for the parent page in the explore index #3057 "Fix: Use specific page model for the parent page in the explore index" make sure this works the same with the API.– Not looking relevant.Remove explorer, page search for users with no page permissions #3068– Check. Hiding the menu item will prevent the explorer from opening, same as before.UI
Tests
No page under the root but other pages further – is this possible?– NoCode quality
Live-reload JS/CSS code Feature request: bundled live-reload for front-end tooling #3022Documentation
Tasks backlog post-merge
Features
Store filter settings in localStorage– Not relevant anymore (removed filter)Tooling
Related work to address later