Add drag-mode switching buttons #57
Conversation
samwilson
force-pushed
the
drag-mode
branch
3 times, most recently
from
240c155
to
d6b2f4a
Compare
assets/styles/app.css
Outdated
| position: absolute; | ||
| right: 30px; | ||
| top: 15px; | ||
| .buttons { |
There was a problem hiding this comment.
I think 'buttons' might be a bit generic. I tried to come up with a better name, but I don't have good ideas at the moment. Perhaps 'output-buttons'?
templates/output.html.twig
Outdated
| <div class="btn-group" role="group"> | ||
| <button type="button" class="btn btn-default active drag-mode drag-mode-crop" | ||
| title="{{ msg('drag-mode-crop') }}" data-drag-mode="crop" disabled> | ||
| <img src="{{ asset('build/images/Crop_-_The_Noun_Project.svg') }}" height="20" width="20" /> |
There was a problem hiding this comment.
Should this have an alt attribute (e.g. alt="Crop") for cross-device compat? Unsure how that would work inside a button, though.
There was a problem hiding this comment.
Done. I think it's fine inside a button.
There was a problem hiding this comment.
As a general comment, I note that the new buttons don't work (obviously) if JavaScript is disabled. What do you think about having a
nojsclass in the html element, and remove it on page ready (like for MW)? Then addnojs .buttons { display: none; }
Great idea, done. Also applied it to the copy-to-clipboard button.
assets/styles/app.css
Outdated
| position: absolute; | ||
| right: 30px; | ||
| top: 15px; | ||
| .buttons { |
templates/output.html.twig
Outdated
| <div class="btn-group" role="group"> | ||
| <button type="button" class="btn btn-default active drag-mode drag-mode-crop" | ||
| title="{{ msg('drag-mode-crop') }}" data-drag-mode="crop" disabled> | ||
| <img src="{{ asset('build/images/Crop_-_The_Noun_Project.svg') }}" height="20" width="20" /> |
There was a problem hiding this comment.
Done. I think it's fine inside a button.
|
I've also updated to @nayoub 's suggestion of starting in move mode, and requiring a click on crop to start cropping. This means that the mode buttons don't start as disabled. |
assets/app.js
Outdated
| @@ -73,9 +77,13 @@ $(function () { | |||
| x = document.querySelector('[name="crop[x]"]'), | |||
There was a problem hiding this comment.
I didn't notice earlier, but now these elements don't exist when there's no output, so there's a JS error when you first load the page (Cannot read property 'value' of null at line 80). I guess the whole cropping thing should be inside a conditional
There was a problem hiding this comment.
Good point, done. Sorry I didn't notice that earlier!
Add radio buttons to toggle between 'crop' and 'move' drag modes, and a separate 'Transcribe area' button for submitting to crop the image. The change to the copy-button is because it's now inside the form. Bug: https://phabricator.wikimedia.org/T288672
|
Thank you, everything looks good. I was thinking for a future change the "transcribe area" button could do its thing via AJAX and show the result without reloading the page, but it's just a minor thing that we don't have to care about for the time being. |
Add radio buttons to toggle between 'crop' and 'move' drag modes,
and a separate 'Transcribe area' button for submitting to crop
the image.
The change to the copy-button is because it's now inside the form.
Bug: https://phabricator.wikimedia.org/T288672