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

Drag and drop screenshots to reorder #1772

Merged
merged 3 commits into from Apr 5, 2019
Merged

Conversation

bartaz
Copy link
Contributor

@bartaz bartaz commented Apr 3, 2019

Fixes #1673

Enables drag and drop to reorder screenshots on publisher listing page.

QA

  • ./run or https://snapcraft-io-canonical-websites-pr-1772.run.demo.haus/
  • go to listing page of any snap with screenshots
  • drag a screenshot to reorder, if you drop it in different place change should be detected (Revert and Save should be enabled)
  • if you drag back to same place (old order) Revert would be disabled again
  • make sure adding screenshots works (and new screenshots can be reordered)
  • make sure removing screenshots work (and dnd works after removing)
  • make screen smaller (so screenshots don't fit into one line), make sure reordering works

dnd

@codecov-io
Copy link

codecov-io commented Apr 3, 2019

Codecov Report

Merging #1772 into master will decrease coverage by 0.08%.
The diff coverage is 83.33%.

Impacted Files Coverage Δ
static/js/publisher/form/media.js 89.09% <0%> (-10.91%) ⬇️
static/js/publisher/form/mediaItem.js 75% <100%> (+3.57%) ⬆️
static/js/publisher/form/mediaList.js 100% <100%> (ø)
webapp/publisher/snaps/logic.py 92.85% <100%> (ø) ⬆️

@webteam-app
Copy link

@Lukewh
Copy link
Contributor

Lukewh commented Apr 3, 2019

When you have no images, upload 5, reorder and save - when the page reloads they're in the upload order. Maybe the inputs need reordering?

@hughesjordan
Copy link
Contributor

Hey @bartaz a few issues:

  • This part doesn't seem to work for me, revert button is still enabled:
  • if you drag back to same place (old order) Revert would be disabled again
  • When I add an image, re-order that image and click on save, it then re-appears in last slot.

  • After saving any changes, when I re-order any image the 'Changes applied' notification does not disappear.

@bartaz
Copy link
Contributor Author

bartaz commented Apr 3, 2019

@Lukewh it seems that our backend doesn't take order to account when it comes to new screenshots. It just adds new screenshots to the end of the list (in order of upload).
While reordering inputs may be a hacky workaround, it will only help if all screenshots are new, not if new ones are mixed between old ones. So I guess it would be better to fix it in our backend logic.

@bartaz
Copy link
Contributor Author

bartaz commented Apr 4, 2019

@hughesjordan @Lukewh Updated so that order of newly added screenshots is correctly preserved.

For other issues:

if you drag back to same place (old order) Revert would be disabled again

I see it working for myself. @hughesjordan Is there any specific case when revert button is enabled if order is same as saved?

After saving any changes, when I re-order any image the 'Changes applied' notification does not disappear.

That is the case also for removing screenshots, etc. So I don't feel like it's an issue with this specific PR.

@hughesjordan
Copy link
Contributor

@bartaz does not seem to be any specific case for me:

Apr-04-2019 9-37-13 am

@bartaz
Copy link
Contributor Author

bartaz commented Apr 4, 2019

@hughesjordan This doesn't seem to be related to screenshots, so I reported it separately (#1772).

Does screenshot reordering work good apart from that?

@hughesjordan
Copy link
Contributor

@bartaz apart from that its good! 👍🏻

Copy link
Contributor

@Lukewh Lukewh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks @bartaz

@bartaz bartaz merged commit c03ae2c into canonical:master Apr 5, 2019
@bartaz bartaz deleted the dnd-to-reorder branch April 5, 2019 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants