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

New explorer using the admin API, and React tooling #3012

Merged
merged 11 commits into from May 13, 2017

Conversation

@thibaudcolas
Member

thibaudcolas commented Sep 19, 2016

New explorer using the admin API, and related React tooling / JS build pipeline / testing tools.

react-explorer

Tasks TODO before merge

  • Detailed list all of the tasks that need to be done before this can be merged (...)
  • Resubmit against master instead of torchbox:admin-api
  • Make WIP available on public wagtaildemo instance so people can easily test it. – https://wagtaildemo-springload.herokuapp.com/admin/

Features

  • Have a way to go to the homepage from the explorer
  • Support for sites with multiple homepages
  • 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 filter
  • "See all pages" menu item after all of the menu items if there are more pages at the current level than is shown. See https://gist.github.com/thibaudcolas/52f3921b1251c8a6bc278f5227713ae3
  • #2869 "Limit explorer menu nav to the subtree the user has permission over" make sure this works the same with the API.
  • #2878 "Adds 'exclude_from_explorer' attribute to ModelAdmin class" make sure this works the same with the API. – Check. Uses construct_explorer_page_queryset, which is about the listing pages not the menu.
  • #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.
  • #3068 – Check. Hiding the menu item will prevent the explorer from opening, same as before.
  • Focus trap for tab navigation
  • Error handling

UI

  • Basic translations using standard Django template tags to make JS variables
  • Date format localization (Look at datepicker. Django probably has some way to share its current locale, and format.)
  • Mobile UI as good as the existing one
  • Matte layer when explorer is open?
  • Fix explorer not closing other panels (settings) when opening
  • If you open the explorer while scrolled down, you can't scroll back up to see the top of the explorer (you have to close explorer, scroll up, and open it again)
  • #2985 "Increase legibility by increasing contrast" make sure this is reflected in the new explorer UI
  • Explorer does not toggle when clicking "Pages" button.

Tests

  • Unit tests of the JS code including React components – 96% coverage?
  • Test on some real-world content to see how it holds up. Test cases:
    • 100+ pages at a given level of the page tree
    • 1000+ pages at a given level of the page tree
    • 10+ levels in the page tree
    • 1 level in the page tree
    • Empty page tree
    • No page under the root but other pages further – is this possible? – No
    • Pages with very long titles (overflowing 2-3 lines each)
  • Cross-browser tests
  • Contrast tests
  • Accessibility tests (VoiceOver navigation, keyboard navigation, focus trap)

Code quality

Documentation

  • React components documentation
  • Redux actions documentation
  • API client documentation

Tasks backlog post-merge

  • Remove dead code – see separate PR #3385

Features

  • Store filter settings in localStorage – Not relevant anymore (removed filter)

Tooling

Related work to address later

  • Node tooling (nvm) doc #2730
@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Sep 19, 2016

@kaedroho especially, I have rebased springload:feature/react-explorer to remove all of the early commits you did in here that were then ported into the admin API work / now merged into master. This branch now only contains changes to the JS/SCSS(/ templates) files. Can you confirm this is what you were after? If so, I think #2297 can be closed.

@jonnyscholes

This all looks brilliant to me! I found the React stuff very readable at a glance - although I havnt gone through it line by line. I mainly reviewed the CSS/Markup - a couple of tiny comments but other than those that all looked good too :)

this.props.init();
document.body.style.overflow = 'hidden';
document.body.classList.add('explorer-open');

This comment has been minimized.

@jonnyscholes

jonnyscholes Sep 21, 2016

Contributor

Should the overflow be put in the CSS to keep it in one place?

This comment has been minimized.

@thibaudcolas

thibaudcolas Sep 22, 2016

Member

👍 Moved the overflow:hidden; to the properties of .explorer-open

background: rgba(0,0,0,0.5);
}
[aria-role='presentation'] {

This comment has been minimized.

@jonnyscholes

jonnyscholes Sep 21, 2016

Contributor

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.

This comment has been minimized.

@thibaudcolas

thibaudcolas Sep 22, 2016

Member

👍 I changed this to simply using the visuallyhidden class that already exists within the codebase.

@kaedroho kaedroho referenced this pull request Sep 21, 2016

Closed

[WIP] Feature: React Explorer #2297

0 of 6 tasks complete
@SalahAdDin

This comment has been minimized.

Contributor

SalahAdDin commented Sep 22, 2016

Awesome man!

@kaedroho

This comment has been minimized.

Member

kaedroho commented Sep 22, 2016

Can you confirm this is what you were after?

Yep, this is exactly what I'm after. Thanks!

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Sep 27, 2016

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 :)

@kaedroho

This comment has been minimized.

Member

kaedroho commented Sep 29, 2016

Just running this locally, noticed a couple of issues:

  • There's currently no way to get to the homepage from the explorer
  • If you open the explorer while scrolled down, you can't scroll back up to see the top of the explorer (you have to close explorer, scroll up, and open it again)
@ababic

This comment has been minimized.

Collaborator

ababic commented Oct 12, 2016

So long as this still respects changes made via the construct_explorer_page_queryset hook, #2878 should be fine. One less thing to worry about :)

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Oct 13, 2016

@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 😁.

@gasman gasman referenced this pull request Dec 22, 2016

Closed

Feature/new explorer #3249

@thibaudcolas thibaudcolas force-pushed the springload:feature/react-explorer-wip branch from c6bb1af to 03e1a00 Jan 25, 2017

@thibaudcolas thibaudcolas force-pushed the springload:feature/react-explorer-wip branch from 03e1a00 to 0303b7f Feb 6, 2017

@thibaudcolas thibaudcolas added this to the 1.10 milestone Feb 10, 2017

@jjanssen

This comment has been minimized.

Member

jjanssen commented Feb 10, 2017

Test findings

  • IE9 - Clicking on Pages (f.k.a. Explorer) triggers a page refresh instead of opening it.
  • IE10 - Clicking on Pages (f.k.a. Explorer) triggers a page refresh instead of opening it.
  • IE11 - Clicking on Pages (f.k.a. Explorer) triggers a page refresh instead of opening it.
  • IE11 - Clicking on Pages (f.k.a. Explorer) triggers a page refresh instead of opening it.
  • iOS8 - Tablet Portrait - Clicking on Pages (f.k.a. Explorer) triggers a page refresh instead of opening it.
  • Safari 10.0.3 (Sierra) - Clicking on the white overlay backdrop does not close the explorer. Whereas in desktop (Chrome) this is working without problems. Note: this is also the case with Settings, something we did not touch.
  • IE9 - No white overlay is shown. So when clicking right from it on the body content it does not close the explorer like it does on other devices.
  • IE9 - Clicking on the Settings does not close the explorer menu.
  • IE9 - Body locking class explorer-hidden looks like it not being active. Scrolling is still possible.
  • IE9 - Scrollbar is off by 20px when inline scrolling in the explorer is enabled.
    image
  • Cross-browser - The overlay appears hard on screen, whereas a soft transition would seem more easing.
  • Cross-browser - Having a message after saving and reopening the explorer causes the overlay not to align to the top completely.
    image
  • When pressing ESC it would make sense to close the explorer? (cc: @thibaudcolas)
  • UX - When having a inline scroll available in the explorer it also takes the View all link to into account. It would make more sense to let it stick to the bottom as we do with the explorer header.
    image
  • UX - We have a chevron in place for indicating there is more to menu feature like we have for settings. We should add a chevron to the Pages item as well to indicate there "is more" (consistency).
    image
  • Mobile - Explorer is wider than a mobile screen (360px vs 320px).
    image
  • Pages with long titles and children overlap through the chevron.
    image

@thibaudcolas thibaudcolas changed the title from [WIP] Feature: React Explorer to New explorer using the admin API, and React tooling Feb 10, 2017

@tomdyson tomdyson referenced this pull request Feb 10, 2017

Closed

Rename 'Explorer' to 'Pages' [WIP] #3360

1 of 3 tasks complete
@jjanssen

This comment has been minimized.

Member

jjanssen commented Mar 9, 2017

Did some additional testing after the latest changes.
Only thing left besides the " Explorer does not toggle when clicking 'Pages' button." The menu item does not have an active state when a URL beneath /pages/ is active.

Currently it shows the following:
image

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Mar 16, 2017

I think the explorer PR is ready to be merged, apart from 3 things where I need help:

  • Reviewing the PR – eyes over the code would be appreciated
  • Cross-browser testing – I will finish this on sunday.
  • Finishing adding the setting to customise the ordering / API behavior – I've added this as a global variable initialised in the templates, but it's not hooked into the Django settings yet (I don't know how to do that)

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

  • Code review
  • UX review
  • Settings flag to user test multiple orderings.

Cross-browser tests

  • - Browser Device/OS Version(s)
  • - Mobile Safari iOS Phone 10 (emulated)
  • - Mobile Safari iOS Phone 9
  • - Mobile Safari iOS Tablet 10 (emulated)
  • - Mobile Safari iOS Tablet 9 (emulated)
  • - Chrome Android 56
  • - Chrome Android 55
  • - IE Desktop 11
  • - Chrome Desktop 56
  • - Chrome Desktop 55
  • - MS Edge Desktop 15
  • - MS Edge Desktop 14
  • - Firefox Desktop 52
  • - Firefox ESR Desktop 45.8
  • - Safari macOS 9
  • - Safari macOS 8

Last issues

  • "Explore all in undefined" in PageCount when the first API call fails.
  • Safari – double scrolling bars
  • The first level of the explorer is not cached (in OPEN_EXPLORER in the nodes reducer, state[payload.id] = Object.assign({}, defaultPageState); should be state[payload.id] = Object.assign({}, state[payload.id], defaultPageState);.
  • PageCount does not display on mobile.
  • Fix failing tests for get_explorable_root_page usage.

Tasks backlog post-merge

  • Remove dead code – see separate PR #3385
  • Update the shrinkwrap

WIP changelog

  • A new explorer! (Cape Town sprint, Ede sprint, Reykjavík sprint)
  • Add get_context method to MenuItem to define context for the template, overridable to use more data. (Thibaud Colas, Matthew Westcott)
@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented Mar 22, 2017

Thanks @robmoorman to put your hand up to review this 😉

@thibaudcolas thibaudcolas requested a review from robmoorman Mar 22, 2017

@thibaudcolas thibaudcolas force-pushed the springload:feature/react-explorer-wip branch 2 times, most recently from 89f25c5 to 00bd19c May 6, 2017

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented May 10, 2017

Removing the --quiet gives a lot more info but frustratingly nothing about this failure. @jjanssen says it works for her. I'll do more troubleshooting locally 😶

Edit: works on Windows 8 / Cygwin, works on OSX Yosemite, works on Ubuntu / Vagrant.

@thibaudcolas thibaudcolas force-pushed the springload:feature/react-explorer-wip branch from 47da13b to eb6c971 May 13, 2017

@thibaudcolas thibaudcolas merged commit b17d11a into wagtail:master May 13, 2017

0 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/drone/pr this build is pending
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@thibaudcolas thibaudcolas deleted the springload:feature/react-explorer-wip branch May 13, 2017

@SalahAdDin

This comment has been minimized.

Contributor

SalahAdDin commented May 14, 2017

This could be a problem with packages that don't use API Integration(wagtail-embed-videos for example).

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented May 14, 2017

@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?

@SalahAdDin

This comment has been minimized.

Contributor

SalahAdDin commented May 15, 2017

I think they need implements API endpoint for this package works with this new admin, isn't it?

@thibaudcolas

This comment has been minimized.

Member

thibaudcolas commented May 15, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment