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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toolbar tool updates #255

Merged
merged 15 commits into from Jun 8, 2019

Conversation

Projects
None yet
3 participants
@sashadev-sky
Copy link
Collaborator

commented May 12, 2019

References #246, publiclab/mapknitter#124 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 馃搼 and links the original issue above 馃敆
  • tests pass -- look for a green checkbox 鉁旓笍 a few minutes after opening your PR -- or run tests locally with grunt
  • code is in uniquely-named feature branch and has no merge conflicts 馃搧
  • screenshots/GIFs are attached 馃搸 in case of UI updates
  • @mention the original creator of the issue in a comment below for help or for a review

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

=====================================================

Updates

  1. Keymapper order re-arranged to match toolbar order

  2. Update RotateScale -> Rotate+Scale in hovertext and keymapping and fix hovertext toggle

  3. Remove deprecated charset attribute from our link tags in index.html and select.html and unnecessary type="text/css" (rel=stylesheet is sufficient)

  4. Switched to material design icons :

    • Offers much more flexibility for customizing and has every icon in solid, outlined, two-toned, rounded, and sharp for free. Font awesome only lets you have their least appealing icons for free
    • Has way more options. Font awesome does not even have rotation icons in their latest release (which would have given us vector scale): https://fontawesome.com/icons?d=gallery&q=rotate so
  5. Fixed _toggleOutline method: fixed bug where we were also toggling image opacity here not just the outline opacity.

  6. _toggleOrder

  • we have a client side drag and drop now in mapknitter, and in LDI I don't think it's ever going to make sense to try to specifically target images z-index with these methods. I think it's a good fix for now to just send it to the top or send it to the back and it works (if you want an image stacked in the middle, send it up above one and send the next up above it).

    • pending confirmation: I can open a new issue to create a cool drag and drop minimap like view?
  • Fixed hotkeys: (these was probably a plan in place to fix sendUp and sendDown to make them more complex but they are the same as _ToggleOrder right now except they don't swap the this._toggledImage property. I use that property to make the tool icon dynamic so changed it back. (also again don't think it'll ever make sense to use a hotkey to target specific z-indexes)

'j': "_sendUp",        -> "_toggleOrder"
'k': "_sendDown",  ->  "_toggleOrder"
  1. bumped leaflet from "leaflet": "^1.0.0" to "leaflet": "^1.5.1" in package.json devDependencies.

=====================================================

=====================================================

pending

  • name of the action in the mode to also be updated to use rotate+scale?

=====================================================

@sashadev-sky sashadev-sky changed the title Toggle rotate/scale hover text Toggle rotate/distort hover text May 12, 2019

@sashadev-sky sashadev-sky requested review from jywarren and rexagod May 12, 2019

@sashadev-sky

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

@jywarren @rexagod The hovertext is fixed:
hovertext

for the icons need your input!

I realized the reason they are confusing is because the current distort icon is an icon that typically represents "rotate". I would say:

step 1: maybe we should swap them 馃憤
step 2: replace the icon that represents an image for this https://fontawesome.com/icons/vector-square?style=solid 馃憤

let me know your thoughts!

@sashadev-sky sashadev-sky changed the title Toggle rotate/distort hover text Toggle rotateScale/distort hover text May 12, 2019

@sashadev-sky sashadev-sky force-pushed the sashadev-sky:hoverText branch from d94c45c to b2f2a27 May 15, 2019

@jywarren

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

This looks super. Shall we call it "Rotate/Scale" instead of "RotateScale"?

@jywarren

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

And vectorscale icon is awesome!

@sashadev-sky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 2, 2019

@jywarren I updated RotateScale to Rotate+Scale in the hovertext and keymapper because it seemed to make the most sense aesthetically, since we might use a slash for something like "Send up / down". What do you think?

Also did you want the name of the action in the mode to also be updated to use rotate+scale? It is currently rotateScale? This would be the name change for the option the user can pass in to set a default tool.

@rexagod

@sashadev-sky sashadev-sky changed the title Toggle rotateScale/distort hover text Toolbar tool updates Jun 2, 2019

@sashadev-sky sashadev-sky force-pushed the sashadev-sky:hoverText branch from 484803a to be22de9 Jun 2, 2019

@sashadev-sky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 4, 2019

@jywarren @rexagod please give this a final review! Lots of changes (see PR description). Here is the UI now:
tools

@rexagod

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

Okay wow! This looks superb! Reviewing this ASAP.

@sashadev-sky sashadev-sky requested review from jywarren and rexagod and removed request for jywarren Jun 4, 2019

@rexagod

rexagod approved these changes Jun 4, 2019

@sashadev-sky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

@jywarren would we be ok to merge this? I will follow up with a PR for documentation updates

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Love this! However, let's get input from Sairam and Mo -- @stefannibrasil where is the best place to ask their input on this icon change?

But yes, otherwise I am very +1 to merging!

@sashadev-sky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

Love this! However, let's get input from Sairam and Mo -- @stefannibrasil where is the best place to ask their input on this icon change?

But yes, otherwise I am very +1 to merging!

Sounds good! I have not met Sairam and Mo yet are they heading UI updates across PL or mapknitter?

Also - i'll do this in a follow up PR if yes but did you want rotateScale to be changed to rotate+scale as a mode too? see #255 (comment)

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@sashadev-sky

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 6, 2019

@jywarren ah ok! I see their usernames! If @stefannibrasil doesn't get back to us I can post a question directed at them and tag them??

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

sashadev-sky added some commits Jun 8, 2019

s

@sashadev-sky sashadev-sky merged commit 2da25be into publiclab:main Jun 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.