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

Api: add iconSrc of installed apps #380

Merged
merged 2 commits into from Sep 5, 2019

Conversation

@sohkai
Copy link
Member

commented Sep 3, 2019

Adds an iconSrc property to any returned app objects from api.installedApps() and api.currentApp() (if the icons are available).

If there are multiple icons declared in an app's manifest.json, this uses the first one for simplicity.

  • I have updated the associated documentation with my changes

@sohkai sohkai requested a review from bpierre Sep 3, 2019

@auto-assign auto-assign bot requested a review from 2color Sep 3, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

I wonder if we shouldn’t also (or only?) expose a .getIconSrc(size) method, doing what getAppIconBySize() does in aragon/aragon?

@sohkai

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

Mmm so you'd give back both the content (either http provider or ipfs provider) and the icons and let the users figure out which one they'd like to use?

@bpierre

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

I think we can use these internally, and let users only set the display size they want − or no parameters at all to get the default size.

<img width="40" height="40" src={app.icon(40)} />
@sohkai

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@bpierre I've adjusted the icon API to be based on the size-based api in 4384998.

For now I think it's OK to copy over the code; it's fairly small and generic code for handling web manifest icons.

@bpierre
bpierre approved these changes Sep 3, 2019
Copy link
Member

left a comment

Yes looking good! Ready for an adaptive icons future 😍

@sohkai sohkai merged commit f2fd061 into master Sep 5, 2019

11 of 12 checks passed

bootstrap bootstrap
Details
build build
Details
install install
Details
lint lint
Details
size size
Details
test test
Details
coverage/coveralls Coverage decreased (-0.3%) to 54.498%
Details
License Compliance All checks passed.
Details
WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the api-apps-icon branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.