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

fix(badge): allow more data types for badge content #20331

Merged
merged 1 commit into from
Aug 20, 2020
Merged

fix(badge): allow more data types for badge content #20331

merged 1 commit into from
Aug 20, 2020

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 16, 2020 •
edited
Loading

Currently the badge's content is limited to string which excludes other legitimate use cases like numbers. These changes turn it into an string | number | undefined | null since we aren't actually doing anything with the value, apart from forwarding it.

Fixes #20326.
@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Aug 16, 2020
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 16, 2020
@jelbourn
Copy link
Member

What would you think about just changing the type to number | string rather than making it generic? The badge is only really intended to show super simple content, so I don't know that it's super valuable to handle the toString() call in the component.
@crisbeto
Copy link
Member Author

I was considering it, but I don't think there's much to gain from the addition type safety since we don't do anything with the content anyway. The advantage I see in keeping it any is that people won't have to add non-null assertions if they bound value is nullable.
@jelbourn
Copy link
Member

jelbourn commented Aug 17, 2020 •
edited
Loading

How about string | number | undefined | null? It feels to me that we'd probably want to discourage people binding other types of values because it's likely not what they really intended.
@crisbeto
Copy link
Member Author

Done.
@crisbeto
fix(badge): allow more data types for badge content
cf058f0 
Currently the badge's content is limited to `string` which excludes other legitimate use cases like numbers. These changes turn it into an `string | number | undefined | null` since we aren't actually doing anything with the value, apart from forwarding it.

Fixes angular#20326.
jelbourn
jelbourn approved these changes Aug 17, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM
@jelbourn jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: merge The PR is ready for merge by the caretaker labels Aug 17, 2020
@jelbourn
Copy link
Member

Caretaker note: this could be breaking if people are reading out of the badge's content, but I have a hard time imagining why people would do that (maaaaaybe tests). If we find that this breaks anything in Google, we should bump it to major and amend the commit to include a breaking change block.
@wagnermaciel wagnermaciel merged commit 75f73ae into angular:master Aug 20, 2020
wagnermaciel pushed a commit that referenced this pull request Aug 20, 2020
@crisbeto @wagnermaciel
fix(badge): allow more data types for badge content (#20331)
c492ba0 
Currently the badge's content is limited to `string` which excludes other legitimate use cases like numbers. These changes turn it into an `string | number | undefined | null` since we aren't actually doing anything with the value, apart from forwarding it.

Fixes #20326.

(cherry picked from commit 75f73ae)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.
@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
4 participants
@crisbeto @jelbourn @googlebot @wagnermaciel