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

Keywords (WIP) #1268

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

Keywords (WIP) #1268

wants to merge 56 commits into from

Conversation

Integral
Copy link
Member

@Integral Integral commented Apr 24, 2019

  • Rename property CustomMetadataField.value to ‘values’ and store there array
  • Update FigurePanelImporter to map keywords to an array of values
  • Update FigurePanelExporter to export the array of values
  • Implement a component for the proposed overlay/popup following the described UX
  • Introduce a command to remove the current value (click on ‘remove’ button should trigger this)
    if the cursor is in the removed value, it should be moved to the next ‘reasonable’ place
  • Make sure that keyboard navigation via UP/DOWN and TAB/SHIFT-TAB allows to navigate through the different values
  • (O): click on value needs to change selection and/or overlayId so that popup is shown
  • Add a keyboard shortcut to remove the current value (CommandOrCtrl+Delete)

Tests:

  • fix existing tests for keyword editing

Mockup:

1QR3hs78dFUY5PiMYqexzU56P06rKKk_0m_w0XGA

@obuchtala
Copy link
Member

@Integral I have implemented this as a prototype, because the required solution is kind of new, while very similar to IsolatedNodes. Let's get everything working, and then see how we could extract a parent class, so that this kind of interface will be easier to implement the next time.

@obuchtala
Copy link
Member

@Integral I tried another approach based on CustomSurface and selections. A little more like IsolatedNodeComponent is working. The implementation is still very hacky.
@michael Before we tidy up, we need to 100% nail down how the UX should be in detail, e.g. keyboard handling, undo/redo, etc.

@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #1268 into master will decrease coverage by 1.08%.
The diff coverage is 38.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1268      +/-   ##
==========================================
- Coverage    89.5%   88.41%   -1.09%     
==========================================
  Files         400      404       +4     
  Lines        9027     9214     +187     
==========================================
+ Hits         8080     8147      +67     
- Misses        947     1067     +120
Impacted Files Coverage Δ
src/article/models/CustomMetadataField.js 100% <ø> (ø) ⬆️
src/article/models/ArticleModelPackage.js 100% <ø> (ø) ⬆️
src/kit/ui/Input.js 33.33% <0%> (-1.15%) ⬇️
src/kit/ui/OverlayMixin.js 90.9% <100%> (+10.9%) ⬆️
src/article/shared/ArticleToolbarPackage.js 100% <100%> (ø) ⬆️
src/kit/ui/Popup.js 100% <100%> (ø)
src/article/converter/jats/FigurePanelConverter.js 98.73% <100%> (+0.01%) ⬆️
src/kit/ui/MultiSelectInput.js 97.61% <100%> (-0.06%) ⬇️
src/article/manuscript/ManuscriptPackage.js 100% <100%> (ø) ⬆️
src/article/shared/FigureMetadataComponent.js 100% <100%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58506ef...0e7268c. Read the comment docs.

@obuchtala
Copy link
Member

After playing around with the current interface we came to the conclusion, that the UI feels somewhat unnatural. The reason is not really clear, probably, because it is more like a 'sheet' or 'table' layout, but the UX is not that of a sheet, more like a form.
We see two options, moving towards a sheet UI, or making the current feel more like a form.
The first, is interesting but too much for this iteration.

The latter we should give a try, but only as polishing for the current prototype. Potentially a problem with the current design could also be, that the popover/dropdown feel is not appropriate for something more complex like this. We could instead use more space for the popup, maybe more like a little modal, position at the same location, but just let it looks less like a tooltip-like popover.

Refinements:
[ ] Remove the 'delete' buttons and instead add a contextual tool (the buttons clutter the UI too much)
[ ] Try to add a bit more spacing and let the fields look more like form fields
[ ] The popup is moving down when keywords are added

Regression after changing directory layout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants