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

Add AppBadge #564

Merged
merged 11 commits into from Sep 4, 2019

Conversation

@sohkai
Copy link
Member

commented Sep 3, 2019

Adds an AppBadge.

See designs from Figma.


Also does a few other things to the other badges, now that I've spent some more time / noticed them:

  • Consolidated disabled and badgeOnly external props so it's just badgeOnly
  • Adjusted spacing for IdentityBadge's 'you' tag, but discussing with @dizzypaty exactly how these tags should look due to sizing discrepancies
  • Added a fallback timer to useImageExists(), to let users know if the image is taking a long time to load
  • Adjusted the AddressField's icon slot so that there's no border applied by default (missed this in #557)

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


Source of app's icon asset, preferrably 24x24. A default app icon will be used if this prop is not provided or if the given src was not loadable.

### `identifier`

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 4, 2019

Member

I wonder if we need the identifier / label distinction at this level, would it be possible to only have label?

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Author Member

Hmm, yes, maybe we can just override the popover's title with a prop like we do in IdentityBadge.

In that case I'd prefer to keep the identifier instead, as that's inherent to an app (rather than a custom label / custom popover title, which is external).

This comment has been minimized.

Copy link
@bpierre

bpierre Sep 4, 2019

Member

Let’s keep them both for now, and see how we are using the component before deciding 👍

@bpierre

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@sohkai LGTM!

| -------- | --------------- |
| `String` | None |

Source of app's icon asset, preferrably 24x24. A default app icon will be used if this prop is not provided or if the given src was not loadable.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 4, 2019

Member

What happens if a user does not want to render an icon but still use this badge? Can we make this boolean?

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Author Member

They should probably use a different badge; AFAIK we don't have a case where this badge is shown without the icon.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 4, 2019

Member

Still, IdentityBadge has the option to not show an icon, wouldn't it make sense to allow for it here too?

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Author Member

IdentityBadge only doesn't display the icon if you don't give it an address, at which point there's no way for it to display an icon. Apps should always have icons, or if not, use the default.

<AppBadge
appAddress="0x281c36aee91…c31ef3fc115"
iconSrc="ipfs.io/ipfs/..."
label="50% 50%"

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 4, 2019

Member

I was thinking of a similar api, but then I figured it would make sense to make this consistent with IdentityBadge that uses customLabel and entity (my first implementation actually used IdentityBadge with a few modifications to show the icon).

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Author Member

I'll likely deprecate customLabel and settle on label as an API, since it'll be more obvious setting this prop is meant to override the badge's label.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 4, 2019

Member

Hmm, I could not find the discussion but I recall my first implementation of the custom label identity badge used label and then it was updated to customLabel, I'm just trying to be consistent here since we've had this conversation before but ok, this is a different use case and label would have been my first option too (if IdentityBadge did not exist).

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Author Member

I was looking back at the discussion in #327 and AFAIK we didn't discuss customLabel; it was just for the popover action's naming.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 4, 2019

Member

Yep, that's where I looked too and couldn't find it, but one of the first commits uses label instead of customLabel so, somewhere in there a discussion happened that got me to change the prop name - maybe it happened offline.

@sohkai sohkai changed the title AppBadge: add README Add AppBadge Sep 4, 2019

@sohkai sohkai marked this pull request as ready for review Sep 4, 2019

@sohkai sohkai changed the base branch from newstyle to babel-plugin Sep 4, 2019

@sohkai sohkai changed the base branch from babel-plugin to master Sep 4, 2019

@sohkai sohkai changed the base branch from master to newstyle Sep 4, 2019

sohkai added 7 commits Sep 3, 2019

@sohkai sohkai force-pushed the appbadge branch from 99db23a to 577c882 Sep 4, 2019

@sohkai sohkai changed the base branch from newstyle to master Sep 4, 2019

@sohkai sohkai changed the base branch from master to newstyle Sep 4, 2019

// Check if a remote image exists.
export function useImageExists(src) {
// Check if a remote image exists and can be loaded within a specific amount of time.
export function useImageExists(src, timeUntilFallback = 50) {

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 4, 2019

Author Member

@bpierre I've opted just to include the fallback timer here, since RemoteImage is pretty redundant if useImageExists() already has this built-in.

On success, I choose to keep the displayFallback state as-is, in case a user wanted to keep displaying the fallback even after success.

src/hooks/useImageExists.js Outdated Show resolved Hide resolved
@bpierre
bpierre approved these changes Sep 4, 2019
Copy link
Member

left a comment

L \o\
G /o/
T /o\
M \o/
sohkai added 2 commits Sep 4, 2019
@sohkai

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

I've just pushed a few final commits to adjust the styling for the designs:

  • Popover action uses 2GU internal padding
  • Use consistent tag sizing for 'you' and custom labels

@sohkai sohkai merged commit 1fee556 into newstyle Sep 4, 2019

2 of 5 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
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the appbadge branch Sep 4, 2019

@sohkai sohkai referenced this pull request Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.