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

Notifications: sprinkle AppBadges all around #983

Merged
merged 23 commits into from Sep 10, 2019

Conversation

@AquiGorka
Copy link
Member

commented Sep 9, 2019

image

image

<AppBadge
appBadge={contractAddress}
badgeOnly
iconSrc={iconSrc}

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 9, 2019

Author Member

@sohkai do I've to use a combination of app.baseUrl + iconSrc?

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 9, 2019

Member

I think so, the iconSrc is only relative to that app's IPFS content (which needs the baseUrl). AppBadge just takes a normal URL and tries to fetch its image.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 9, 2019

Author Member

Done. Icons render correctly now:
image

@bpierre

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@AquiGorka isn’t the dropdown heading missing in the first screenshot?

@AquiGorka

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

@bpierre yes, because it was never there to begin with let's add one 👍

@AquiGorka

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Updated as discussed with @dizzypaty :

Custom label > App name + identifier (if more than 1 instance) > App name

image

image

This includes @bpierre 's header 😄

@AquiGorka AquiGorka force-pushed the feature/notifications-app-badges branch from 11b32c9 to 195a44d Sep 10, 2019

@now now bot requested a deployment to staging Sep 10, 2019 Abandoned

@AquiGorka AquiGorka requested a review from bpierre Sep 10, 2019

@bpierre
Copy link
Member

left a comment

LGTM

label ? <LocalLabelPopoverTitle label={label} /> : undefined
}
/>
{!onlyOneInstance && !noIdentifier && !name && (

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 10, 2019

Member

I'd really like to not have the identifier bundled with the AppBadge; we should do this separately outside of the component when we need it

@@ -0,0 +1,26 @@
import React from 'react'

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 10, 2019

Member

It'd be nice if the LocalIdenittyBadge also used these components; there's some subtle differences.

@luisivan luisivan merged commit 1c02483 into master Sep 10, 2019

8 of 9 checks passed

build build
Details
install install
Details
lint lint
Details
now Deployment has failed
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

@delete-merged-branch delete-merged-branch bot deleted the feature/notifications-app-badges branch Sep 10, 2019

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