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.

Sign up for GitHub

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

feat: support badge number on web #4562

Merged
merged 17 commits into from
Dec 4, 2019
Merged

feat: support badge number on web #4562

merged 17 commits into from
Dec 4, 2019

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Jun 12, 2019 •
edited
Loading

Why

Badge numbers were not supported on web so far.

How

This solution uses https://www.npmjs.com/package/@approvals-cloud/badgin which is a wrapper around the future Badging API that nicely falls back to the favicon or title.

Test Plan

Use getBadgeNumberAsync and setBadgeNumberAsync on web.

@EvanBacon EvanBacon self-requested a review June 14, 2019 22:08
@EvanBacon EvanBacon self-assigned this Jun 14, 2019
@EvanBacon EvanBacon added Notifications Platform: web Using Expo in the browser labels Jun 14, 2019
sjchmiela
sjchmiela requested changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@sjchmiela sjchmiela left a comment

Choose a reason for hiding this comment

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

Apart from that one comment looks good to me!

packages/expo/package.json Outdated Show resolved Hide resolved
@sjchmiela
Copy link
Contributor

Could you please also add the CHANGELOG entry for this feature? 🙂

@jaulz
Copy link
Contributor Author

jaulz commented Nov 21, 2019

Sure, will do it by end of next week since I am currently on vacation 😊

@jaulz jaulz requested review from ide and tsapeta as code owners December 3, 2019 08:45
@jaulz
Copy link
Contributor Author

jaulz commented Dec 3, 2019

@sjchmiela I just implemented your suggestions. Anything else? 😊

ide
ide requested changes Dec 3, 2019
View reviewed changes
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

This looks OK to try for now but since a lot of the functionality is in the package if there are issues we might need to revert.

packages/expo/src/Notifications/ExponentNotifications.web.ts Outdated
},
async setBadgeNumberAsync(badgeNumber: number): Promise<void> {
currentBadgeNumber = badgeNumber
badgin.set(badgeNumber)
Copy link
Member

Choose a reason for hiding this comment

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

Does this clear when set to zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does clear the badge if set to zero. I just released a new version that explicitly checks this case.

packages/expo/src/Notifications/ExponentNotifications.web.ts Show resolved Hide resolved
packages/expo/src/Notifications/ExponentNotifications.web.ts Outdated Show resolved Hide resolved
packages/expo/src/Notifications/ExponentNotifications.web.ts Outdated Show resolved Hide resolved
packages/expo/package.json Outdated Show resolved Hide resolved
ide
ide reviewed Dec 3, 2019
View reviewed changes
yarn.lock Outdated Show resolved Hide resolved
@ide
Copy link
Member

ide commented Dec 3, 2019

Looks good, just one more CI test that needs to pass:

🔍 Checking the expo package ...
🏃‍♀️ Running `yarn clean` ...
🏃‍♀️ Running `yarn build` ...
The following build files need to be rebuilt and committed:
build/Notifications/ExponentNotifications.web.d.ts
build/Notifications/ExponentNotifications.web.js
build/Notifications/ExponentNotifications.web.js.map

@jaulz
Copy link
Contributor Author

jaulz commented Dec 4, 2019

Ah, I didn't know that the build files also need to be committed. Checking all the test results I think the PR is finally ready. As far as I could see the failed tests are not related to these changes.

@ide ide merged commit 0ea2028 into expo:master Dec 4, 2019
@jaulz jaulz deleted the patch-3 branch December 4, 2019 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ide ide ide requested changes

@EvanBacon EvanBacon EvanBacon requested changes

@sjchmiela sjchmiela sjchmiela requested changes

@tsapeta tsapeta Awaiting requested review from tsapeta tsapeta is a code owner

Assignees

@EvanBacon EvanBacon

Labels
Notifications Platform: web Using Expo in the browser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jaulz @sjchmiela @ide @EvanBacon