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

Jump to bottom

Make @uppy/unsplash production ready #3196

Merged
merged 25 commits into from Oct 1, 2021
Merged

Make @uppy/unsplash production ready #3196

merged 25 commits into from Oct 1, 2021

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Sep 14, 2021 •
edited

Fixes #3175
Fixes #2635
Fixes #2772

You can use the referenced commits to view the changes for the feature/fix without the refactor noise

  • Refactor most files of the plugin to make things more maintainable and intention revealing
  • Fix photos always being disabled (b1359bd)
  • Add docs to the website with docs/unsplash.md
  • Make the plugin brand compliant
    • Call download attribution endpoint in companion (cbdab4d)
    • Show author of image in UI (5067aea)
      • Show author on image hover
      • Always show author on touch devices
      • Show author in dashboard file overview
Screen.Recording.2021-09-14.at.17.11.49.mov

Todo:

  • Fix tests
  • Show empty screen when there are no results
  • Use app referral in author links (?utm_source=your_app_name&utm_medium=referral)
  • Refactor to make attribution changes across plugins more maintainable
  • Add attribution in the dashboard file overview

@Murderlon Murderlon marked this pull request as draft September 14, 2021 15:28
@Murderlon Murderlon marked this pull request as ready for review September 15, 2021 10:18
@Murderlon
Copy link
Member Author

@nqst note that the video shows the underline on hover but I now changed it to be always present. This better indicates it's a link, not a label.

@nqst
Copy link
Contributor

nqst commented Sep 16, 2021

@nqst note that the video shows the underline on hover but I now changed it to be always present. This better indicates it's a link, not a label.

I think I liked the first version more: when there's an immediate underline, it draws more attention to the author name than before — while it's better to keep that attention on the photos, so that it's easier to select them without distractions.

What do you think?

@Murderlon Murderlon marked this pull request as draft September 16, 2021 15:18
@Murderlon
Copy link
Member Author

Update: we need to add attribution in other places of the UI as well. This will require some refactoring first. The PR will get big but I'll reference the individual commits with the changes that matter.

@Murderlon Murderlon mentioned this pull request Sep 20, 2021
Merged
@Murderlon
Merge branch 'main' into unsplash
dcaf200 
* main:
  Show all details on mobile when `showProgressDetails` is `true` (#3174)
  @uppt/xhr-upload: fix `this.uppy is undefined` error (#3207)
  ci: test on Node.js v16.x (#3205)
  @uppy/dashboard: fix linter (#3206)
  Add `2.1.1` to `CHANGELOG.md`
  Release
  Release
  Fix "attempted to use private field on non-instance" in `SearchProvider` (#3201)
  Add 'done' to `nb_NO.js` (#3200)
  @uppy/transloadit: fix unhandledPromiseRejection failures (#3197)
  fix: AbortController is not defined on Node.js (Server Side Render) (#3169)
@Murderlon
Add author attribution in dashboard file overview
6c2a4c3
@Murderlon
Copy link
Member Author

I added author attribution to the dashboard file overview (6c2a4c3). I made sure the file name always gets truncated to fit on one line so the second line is always the author link, which is also truncated to fit on that line only.

This is the design I came up with. Feel free to provide feedback on it.

Screenshot 2021-09-20 at 18 22 53

@Murderlon Murderlon marked this pull request as ready for review September 21, 2021 10:57
@arturi
Copy link
Contributor

arturi commented Sep 27, 2021

Haven’t tested locally, but watched the video and screenshots, very well done on the implementation, I really like it! Also kudos for passing the name along in tagFile.meta.author, was about to suggest it only to see it’s already in place. Feel free to merge, but I will give a test drive when back from holiday.

@nqst
Copy link
Contributor

nqst commented Sep 28, 2021

I propose to slightly update the attribution design, so it clearly shows that a photo has been imported from Unsplash:

image

Their guidelines require it (your application must attribute Unsplash, the Unsplash photographer [...]).
In addition, I removed the by’s to simplify the i18n.

What do you think?

nqst
nqst approved these changes Sep 28, 2021
View reviewed changes
Copy link
Contributor

@nqst nqst left a comment

Choose a reason for hiding this comment

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

👍

@Murderlon Murderlon merged commit b7559ac into main Oct 1, 2021
@Murderlon Murderlon deleted the unsplash branch October 1, 2021 14:32
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
@Murderlon @nqst @aduh95
Make @uppy/unsplash production ready (transloadit#3196)
9e3067d 
Co-authored-by: Alexander Zaytsev <nqst@users.noreply.github.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nqst nqst nqst approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unsplash: All photos are disabled Add Caption to images from Unsplash when using @uppy/unsplash
4 participants
@Murderlon @nqst @arturi @aduh95