#164878049 Disable contextmenu on H mark image #707

Luleherll · 2019-08-21T16:15:15Z

PT Story: H-mark image: users are right-clicking to just save it; it's not the customized version

https://www.pivotaltracker.com/n/projects/1904891/stories/164878049

Changes proposed in this pull request:

Disable right-clicking on the H-mark
Add a custom context menu
Translate tooltip message
Change H-mark download buttons' color and layout
Write tests

Screenshots (Optional):

Ready for review:
@weedySeaDragon @thesuss @patmbolger

app/views/companies/_company_h_brand.html.haml

- disable contextmenu - add a tooltip on the image [Finishes #164878049]

weedySeaDragon

Change the title of the PR to start with "WIP" to show that this is a "Work In Progress" -- that more work will be coming after you get feedback. I don't know if we discussed this; I likely didn't mention this to you.
- need to get feedback from @thesuss and @patmbolger to see if they like this approach and if so, have suggestions for the specific wording in the tooltip
add these to your list of changes that will need to be included in this PR:
1. cucumber test(s)
2. change CSS so that this tooltip (.tooltip-text ) has the same style as the others (fas_tooltip(...))

patmbolger · 2019-08-21T20:54:53Z

Initial comments (in addition to @weedySeaDragon 's):

The download test is on right of the image (not the left)
The tooltip message has to be translated to Swedish (and make sure that is shown when the language is set to that)
I wonder if we should only show that tooltip when the user right right-clicks on the image? It might be slightly less annoying that way, although it may not be worth worrying about.
I'm wondering if - instead of blocking the entire context menu, and using the tooltip - if we could just remove the menu items related to the image (save, download, get address, etc.).
- if this is a lot of work (including browser-specific code) then it's not worth it,
- as a developer, it would be nice to be able to access the "Inspect Element" option, but that is also not worth a lot of work.
If we're going to do this, it makes sense to also apply the same treatment to the proof-of-membership(?)

patmbolger

Pls see comments.

app/views/companies/_company_h_brand.html.haml

rmzse · 2019-08-22T17:32:15Z

One thing about the text design is to make the purpose clearer by cleaning it up. Make the buttons stand out as clear CTA's should and make the text shorter. A good way to improve UX is to show users what they can do and why.

Changing the text to "Download Image" and "Copy Link to Image" is a good start, check out the image.

The idea is to have the design work for you so you don't have to explain so much, which in turn makes intructions shorter and more compact. Also, by removing the CTA's from to outside of the text paragraph it is easier for the user to really understand what to do instead of insisting to right-click the image.

A better UX would actually be to have the image rendered (once) and the setup of the business through ImageMagick.

- make context message show onclick only - disable contextmenu on 'proof of membership' image - Change CTA buttons layout [#164878049]

app/views/users/_proof_of_membership.html.haml

app/views/companies/_company_h_brand.html.haml

- make context message show onclick only - disable contextmenu on 'proof of membership' image - Change CTA buttons layout [#164878049]

weedySeaDragon

I have some thoughts on layout and style, but want to wait to hear the feedback from @patmbolger and @thesuss about what they think of this in general.

Also -- I have the cucumber tests written and passing. I did need to add a cucumber step to handle right-clicking. Will post about that later.

config/database.yml

config/locales/en.yml

app/assets/stylesheets/company-h-brand.css

- create custom-menu file - fix custom context bug - update yaml files

- add tests - add translation

weedySeaDragon · 2019-09-03T17:28:34Z

config/database.yml

@@ -9,14 +9,10 @@ default: &default
 development:
  <<: *default
  database: shf_project_development


weedySeaDragon

Good work! It's getting there.

Now it's clear enough for me to realize that we need to change the functionality so that the button actually copies the URL to the clipboard -- which is different from what it was doing before. See my comments in the app/views/users/show.html.haml file.

Please also paste a new screenshot into the PR description so that we can see the latest version (since it's now been improved so much).

weedySeaDragon · 2019-09-03T17:29:20Z

config/locales/en.yml

-                            Link to it from your website and use it in social media.
-                            Or Print to use in a physical context, for example, to show a customer.
+      image_how_to_use_html: You can download the image, take a screenshot,
+                            link to it from you website and use it in social media,


"you" should be "your"

weedySeaDragon · 2019-09-03T17:47:19Z

config/locales/sv.yml

@@ -788,7 +788,8 @@ sv:
      use_this_image_link_html: "<span style='font-weight: bold'>
                                Använd den här länken på en extern webbplats:
                                </span>"
-      show_image: Visa bild
+      show_image: Kopiera bildadress


Don't change this one because it makes the meaning of the key (show_image ) different from the meaning of the value (copy the url).

Instead, add a new entry (line) below (you'll need to ensure that the indentation is correct):

copy_image_url: Kopiera bildadress

Likewise for the English translation file (en.yml), add the same line but with the English words: copy_image_url: Copy the URL

Then you need to be sure to use the users.show.copy_image_url translation key instead of the old one (old = users.show.show_image).
(That includes places in cucumber tests.)

weedySeaDragon · 2019-09-03T17:48:03Z

app/views/users/show.html.haml

@@ -90,10 +90,10 @@
        %p= t('.proof_of_membership_describe')
        = link_to(t('.download_image'),
                                   proof_of_membership_path(@user, render_to: :jpg),
-                                   class: 'btn btn-sm btn-secondary')
+                                   class: 'btn btn-sm btn-primary')
        = link_to(t('.show_image'),


We now need to change the functionality: The action of this button needs to (1) get the short url for the proof of membership, (2) copy it to the user's clipboard, and then (3) show a message to the user that the information has been copied to their clipboard and that they can now paste it where needed.

Just glancing quickly at the code (you should look more carefully at it to verify), seems that you need to use the short_proof_of_membership_url(user) method to get the url.

We'll need new text in the locale files for (3) above. See my review comments in the sv.yml file.

We'll need the same stuff for the Company H-markt (need to copy the URL to the clipboard instead of showing the image and add the entries to the locale files)

features/step_definitions/basic_steps.rb

patmbolger · 2019-09-05T15:04:07Z

Looking at the latest version of the PR:

I think we need a little space after the blue buttons - set the bottom margin the same as the top margin.

When I click "download image", this is what I get:

It shouldn't have that text at the top.

"Copy link of the image" does not actually do that - it goes to a page where the user can see how the image renders, get it's size, and also get a shortened URL if they want to use that on the own website. So, either we change the wording ("View Image", or similar), or we change the action.

We should discuss at our next meeting and get consensus on how we want this to work.

- implement copy_to_clipboard button - write logic for contextmenu actions - update test [Finishes #164878049]

houndci-bot · 2019-09-12T16:32:21Z

app/views/companies/_custom_context_menu.html.haml

+%ul.custom-menu
+  %li{"data-action" => "download"}= t('users.show.download_image')
+  %li{"data-action" => "link"}= t('users.show.copy_image_url')
+  %li{"data-action" => "show"}= t('users.show.show_image')


Hash attribute should start with one space after the opening brace
Hash attribute should end with one space before the closing brace
Files should end with a trailing newline

houndci-bot · 2019-09-12T16:32:21Z

app/views/companies/_custom_context_menu.html.haml

@@ -0,0 +1,4 @@
+%ul.custom-menu
+  %li{"data-action" => "download"}= t('users.show.download_image')
+  %li{"data-action" => "link"}= t('users.show.copy_image_url')


Hash attribute should start with one space after the opening brace
Hash attribute should end with one space before the closing brace

app/views/companies/_custom_context_menu.html.haml

app/assets/stylesheets/users.scss

app/assets/stylesheets/style.scss

patmbolger

The code looks very nice! I have just one remaining comment - other than that I am fine with the PR.

app/assets/javascripts/users.js

add some code

13c3f97

houndci-bot reviewed Aug 21, 2019

View reviewed changes

app/views/companies/_company_h_brand.html.haml Outdated Show resolved Hide resolved

ft(H-mark image): disable contextmenu on the H-mark image

9194401

- disable contextmenu - add a tooltip on the image [Finishes #164878049]

Luleherll force-pushed the ft-disable-contextmenu-on-H-mark-image-164878049 branch from 745ac79 to 9194401 Compare August 21, 2019 16:16

weedySeaDragon requested changes Aug 21, 2019

View reviewed changes

patmbolger reviewed Aug 21, 2019

View reviewed changes

Luleherll changed the title ~~#164878049 Disable contextmenu on H mark image~~ [WIP] #164878049 Disable contextmenu on H mark image Aug 22, 2019

thesuss reviewed Aug 22, 2019

View reviewed changes

app/views/companies/_company_h_brand.html.haml Outdated Show resolved Hide resolved

ft(H-markt): Change CTA buttons layout

863b6fd

- make context message show onclick only - disable contextmenu on 'proof of membership' image - Change CTA buttons layout [#164878049]

houndci-bot reviewed Aug 26, 2019

View reviewed changes

app/views/users/_proof_of_membership.html.haml Outdated Show resolved Hide resolved

app/views/companies/_company_h_brand.html.haml Outdated Show resolved Hide resolved

app/views/companies/_company_h_brand.html.haml Outdated Show resolved Hide resolved

Luleherll added 2 commits August 26, 2019 16:04

ft(H-markt): Change CTA buttons layout

4107394

- make context message show onclick only - disable contextmenu on 'proof of membership' image - Change CTA buttons layout [#164878049]

Git pull

f8b7c0f

Luleherll force-pushed the ft-disable-contextmenu-on-H-mark-image-164878049 branch from 62f3d30 to f8b7c0f Compare August 26, 2019 13:34

Add new line

ae56844

weedySeaDragon requested changes Aug 26, 2019

View reviewed changes

config/database.yml Outdated Show resolved Hide resolved

config/locales/en.yml Outdated Show resolved Hide resolved

config/locales/en.yml Outdated Show resolved Hide resolved

app/assets/stylesheets/company-h-brand.css Outdated Show resolved Hide resolved

Luleherll added 2 commits September 2, 2019 12:17

ft(H-markt): change download buttons color

b9ffac1

- create custom-menu file - fix custom context bug - update yaml files

ft(H-markt): writes tests for the functionality

991a678

- add tests - add translation

Luleherll changed the title ~~[WIP] #164878049 Disable contextmenu on H mark image~~ #164878049 Disable contextmenu on H mark image Sep 3, 2019

weedySeaDragon reviewed Sep 3, 2019

View reviewed changes

weedySeaDragon requested changes Sep 3, 2019

View reviewed changes

Remove white space in parameter types to make tests pass

624c95f

houndci-bot reviewed Sep 5, 2019

View reviewed changes

features/step_definitions/basic_steps.rb Show resolved Hide resolved

Luleherll added 2 commits September 9, 2019 11:42

create custom context partial

64920da

ft(h-markt): implement custom contextmenu

196e942

- implement copy_to_clipboard button - write logic for contextmenu actions - update test [Finishes #164878049]

houndci-bot reviewed Sep 12, 2019

View reviewed changes

Change swedish translation and add bottom margin on image action buttons

38e6940

houndci-bot reviewed Sep 30, 2019

View reviewed changes

app/assets/stylesheets/users.scss Show resolved Hide resolved

app/assets/stylesheets/style.scss Show resolved Hide resolved

patmbolger requested changes Sep 30, 2019

View reviewed changes

app/assets/javascripts/users.js Outdated Show resolved Hide resolved

change to

c0f8e92

patmbolger approved these changes Sep 30, 2019

View reviewed changes

weedySeaDragon merged commit 3af0fd1 into AgileVentures:develop Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#164878049 Disable contextmenu on H mark image #707

#164878049 Disable contextmenu on H mark image #707

Luleherll commented Aug 21, 2019 •

edited

Loading

weedySeaDragon left a comment

patmbolger commented Aug 21, 2019

patmbolger left a comment

rmzse commented Aug 22, 2019 •

edited

Loading

weedySeaDragon left a comment

weedySeaDragon Sep 3, 2019

weedySeaDragon left a comment

weedySeaDragon Sep 3, 2019

weedySeaDragon Sep 3, 2019

weedySeaDragon Sep 3, 2019

weedySeaDragon Sep 3, 2019

patmbolger commented Sep 5, 2019

houndci-bot Sep 12, 2019

houndci-bot Sep 12, 2019

patmbolger left a comment

#164878049 Disable contextmenu on H mark image #707

#164878049 Disable contextmenu on H mark image #707

Conversation

Luleherll commented Aug 21, 2019 • edited Loading

PT Story: H-mark image: users are right-clicking to just save it; it's not the customized version

weedySeaDragon left a comment

Choose a reason for hiding this comment

patmbolger commented Aug 21, 2019

patmbolger left a comment

Choose a reason for hiding this comment

rmzse commented Aug 22, 2019 • edited Loading

weedySeaDragon left a comment

Choose a reason for hiding this comment

weedySeaDragon Sep 3, 2019

Choose a reason for hiding this comment

weedySeaDragon left a comment

Choose a reason for hiding this comment

weedySeaDragon Sep 3, 2019

Choose a reason for hiding this comment

weedySeaDragon Sep 3, 2019

Choose a reason for hiding this comment

weedySeaDragon Sep 3, 2019

Choose a reason for hiding this comment

weedySeaDragon Sep 3, 2019

Choose a reason for hiding this comment

patmbolger commented Sep 5, 2019

houndci-bot Sep 12, 2019

Choose a reason for hiding this comment

houndci-bot Sep 12, 2019

Choose a reason for hiding this comment

patmbolger left a comment

Choose a reason for hiding this comment

Luleherll commented Aug 21, 2019 •

edited

Loading

rmzse commented Aug 22, 2019 •

edited

Loading