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

Field guide modal #637

Merged
merged 23 commits into from
Apr 8, 2019
Merged

Field guide modal #637

merged 23 commits into from
Apr 8, 2019

Conversation

srallen
Copy link
Contributor

@srallen srallen commented Apr 1, 2019

Package:

Closes #391.

Describe your changes:
Adds the UI for the field guide

Review Checklist

General

  • Are the tests passing locally and on Travis?
  • Is the documentation up to date?
  • Is the changelog updated?

Apps

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you rm -rf node_modules/ && npm install and app works as expected?

@srallen srallen added the enhancement New feature or request label Apr 1, 2019
@srallen srallen changed the title [WIP] Field guide modal Field guide modal Apr 2, 2019
@srallen
Copy link
Contributor Author

srallen commented Apr 2, 2019

My project 1233 has a field guide. The default project loaded by the classifier does not.

@srallen
Copy link
Contributor Author

srallen commented Apr 2, 2019

I don't have this setup for the light/dark themes yet. This is an already large PR so I'll note in an issue.

@srallen
Copy link
Contributor Author

srallen commented Apr 2, 2019

This also has the issue of the size of the modal changing between the full list of items and selected item. I haven't come up with a better way to have it be responsive and have a max-width yet...

@eatyourgreens
Copy link
Contributor

Testing this out in VoiceOver, I'm wondering if the item buttons should be links to the individual items? Give it a go and let me know what you think. When I press a button, it's not immediately obvious that I've navigated to the item, although I can continue navigating with VoiceOver and read the new text.

There's an annoying jump in height between an item loading and the linked image loading. Can we show a placeholder for images, or set a default height on that box.

@srallen
Copy link
Contributor Author

srallen commented Apr 3, 2019

The new suggested changes feature is neat.

@eatyourgreens
Copy link
Contributor

Yeah. I haven't tried it out for changes that are more than a few characters. Big changes might break things.

@srallen
Copy link
Contributor Author

srallen commented Apr 3, 2019

I updated to use an anchor instead and I like it a lot better for the hover state than what I came up with for the button.

@eatyourgreens
Copy link
Contributor

VoiceOver says there's a missing counterpart translation in the link text somewhere.
Screenshot of the field guide item links, showing a missing translation error for the link text.

@srallen
Copy link
Contributor Author

srallen commented Apr 3, 2019

@eatyourgreens Oops! That's fixed in the latest commit.

@eatyourgreens
Copy link
Contributor

I'm also getting setActiveItem is not a function when I click those links.

@srallen
Copy link
Contributor Author

srallen commented Apr 3, 2019

In addition to the refactor to use Anchor instead of Button for the items list, I've:

  • Created an icon component that uses a placeholder fallback consistently between the list items view and the item details view
  • Added Grommet's ResponsiveContext component to set the widths for the outer Box container. This fixes it between the item list view and item detail view and also makes it responsive. Note Enzyme does not yet support React's createContext, so I cannot test how FieldGuide works until Enzyme updates this. See: Dive shallow in a Context Consumer enzymejs/enzyme#1908
  • Refactored how I was using Grid. It was just wrong before. I'm still learning!
  • Finished the fix for renaming the setActiveItemIndex and activeItemIndex
  • Fix the tests

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

Sorry to be a pain about naming but I think the API for setting the active item is still confusing. It should be clear whether the model expects to store the selected item, or its array index (whichever is most convenient to pass in from the React component.) The code seems to use both terms interchangeably at the moment. I'm bringing this up because it's been a maintenance problem in PFE where we've used eg. workflow interchangeably to refer to either a workflow object or a workflow ID. I think the model API should be either setActiveIndex(index) or setActiveItem(item) but not setActiveIndex(item).

The item links are all showing up as visited links, so they could use unique URLs for the individual items if that's easy to implement.

Testing this out with the keyboard, there's a weird focus problem that might be a Grommet bug. Activate the field guide button from the keyboard. If you press Esc the field guide closes, as expected. Open it again, from the keyboard, and tab to a link. Activate the link with Enter. I'm finding that Esc doesn't do anything from the item page and I have to close the field guide with the mouse. Grommet might be losing the keyboard focus when the guide's content changes.

@srallen
Copy link
Contributor Author

srallen commented Apr 4, 2019

The focus bug I'll open as a separate issue. It might be a Grommet bug.

@srallen srallen mentioned this pull request Apr 4, 2019
@eatyourgreens
Copy link
Contributor

I've pulled down the rebased branch and the item links aren't working any more. I'll have a look to see if I can figure out why.

Copy link
Contributor

@eatyourgreens eatyourgreens left a comment

Choose a reason for hiding this comment

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

Only the last link in the item menu can be clicked. The others don't work.

packages/lib-classifier/src/store/FieldGuideStore.js Outdated Show resolved Hide resolved
@eatyourgreens eatyourgreens self-assigned this Apr 5, 2019
@eatyourgreens
Copy link
Contributor

Just waiting on Travis.

@eatyourgreens eatyourgreens merged commit 59ac933 into master Apr 8, 2019
@eatyourgreens eatyourgreens deleted the field-guide-modal branch April 8, 2019 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants