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.

Sign up for GitHub

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

Jump to bottom

Toolbar tool updates #255

Merged
merged 15 commits into from
Jun 8, 2019
Merged

Toolbar tool updates #255

merged 15 commits into from
Jun 8, 2019

Conversation

sashadev-sky
Copy link
Member

@sashadev-sky sashadev-sky commented May 12, 2019 •
edited
Loading

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 02:24
@sashadev-sky sashadev-sky mentioned this pull request May 12, 2019
Closed
@sashadev-sky
Copy link
Member Author

sashadev-sky commented May 12, 2019 •
edited
Loading

@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 mentioned this pull request May 12, 2019
Closed
@sashadev-sky sashadev-sky changed the title Toggle rotate/distort hover text Toggle rotateScale/distort hover text May 12, 2019
@jywarren
Copy link
Member

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

@jywarren
Copy link
Member

And vectorscale icon is awesome!

@sashadev-sky
Copy link
Member Author

@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
Copy link
Member Author

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

@rexagod
Copy link
Member

rexagod 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 June 4, 2019 18:41
@rexagod rexagod mentioned this pull request Jun 5, 2019
Closed
@sashadev-sky
Copy link
Member Author

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

@jywarren
Copy link
Member

jywarren 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
Copy link
Member Author

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
Copy link
Member

jywarren commented Jun 6, 2019 via email

@jywarren
Copy link
Member

jywarren commented Jun 6, 2019 via email

@sashadev-sky
Copy link
Member Author

@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
Copy link
Member

jywarren commented Jun 6, 2019 via email

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

@jywarren @rexagod Mo said he liked the update so I will merge now :)
https://publiclab.org/questions/molangmuir10/05-30-2019/how-can-i-download-pi-builder-sd-card-image-with-the-wifi-network-for-camera-access-infragram-and-image-sequencer#answer-0-comment

@sashadev-sky sashadev-sky mentioned this pull request Jun 8, 2019
Open
@sashadev-sky sashadev-sky mentioned this pull request Jun 18, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rexagod rexagod rexagod approved these changes

@jywarren jywarren Awaiting requested review from jywarren

Assignees
No one assigned
Labels
bug enhancement ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sashadev-sky @jywarren @rexagod