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

feat: load app icon from new icon schema with size selector #691

Merged
merged 8 commits into from Apr 15, 2019

Conversation

Projects
None yet
2 participants
@sohkai
Copy link
Member

commented Apr 11, 2019

Apps are now conforming to the icons schema from web manifests, so they can package multiple different icon types.

sohkai added some commits Apr 11, 2019

@sohkai sohkai requested a review from bpierre Apr 11, 2019

: null
if (app && app.baseUrl && Array.isArray(app.icons)) {
const iconSize =
app.icons.find(({ sizes }) => sizes === '22x22') || app.icons[0]

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 12, 2019

Member

What would you think about doing something like this:

  • Parse the sizes available in app.icons (parseInt() should be enough to get the width).
  • Pass the size at which we want to render the icon to appIconUrl(), as an optional parameter (a single number is enough since icons are square).
  • If a size is provided, send the closest icon which size is higher or equal.
  • If no size is provided, or the closest size higher or equal can’t be found, send the largest icon available.
@@ -15,9 +15,12 @@ export function compose(...funcs) {

// Get the icon URL of an app
export function appIconUrl(app) {
return app && app.baseUrl
? resolvePathname('images/icon.svg', app.baseUrl)

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 15, 2019

Author Member

Ahh shoot, we should also provide a fallback to fetch the old icon so they don't get broken by this change.

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 15, 2019

Member

Ah yes, we could add a legacyAppIconUrl() function to do that, and add another RemoteImage here, in case the first one is not found: https://github.com/aragon/aragon/blob/master/src/components/AppIcon/AppIcon.js#L59

bpierre and others added some commits Apr 15, 2019

@sohkai

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

@bpierre I've just added 516937e, so we grab the last item in the sorted sizes rather than the last item in icons (which may not be sorted).

@sohkai sohkai changed the title feat: load app icon from new icon schema feat: load app icon from new icon schema with size selector Apr 15, 2019

@sohkai sohkai merged commit 8f52fb2 into master Apr 15, 2019

1 of 4 checks passed

License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the load-app-icon branch Apr 15, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented on 516937e Apr 15, 2019

Oh yes 🤦‍♂️

2color added a commit that referenced this pull request Apr 16, 2019

Merge remote-tracking branch 'origin/master' into activity-panel
* origin/master:
  fix: align connect account with preferences button (#700)
  Fix import file button (#698)
  feat: add current account with connect account button (#693)
  feat: load app icon from new icon schema with size selector (#691)
  Discover Apps: update Espresso + Payroll icons (#696)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.