fix(callout): allow Callout component to not have an icon #717
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Callout component can now exist sans icon by adding icon={false} as a prop
re 577
|
LGTM but i will feel better after @Timmy or @masoudmanson Take a look |
tihuan
reviewed
Nov 29, 2023
| @@ -56,7 +56,7 @@ const Callout = ({ | |||
| }; | |||
|
|
|||
| const getIcon = () => { | |||
| if (icon) return icon; | |||
| if (icon !== undefined) return icon; | |||
There was a problem hiding this comment.
Niice! Thanks so much for the fix, @liaprins 🙏 ❤️
Can we add a comment to explain why we explicitly use icon !== undefined here please? In case someone in the future "refactors" this back to if (icon) return icon lol
Something like:
/**
* (liaprins): Explicitly use `icon !== undefined` to fix bug in ticket: https://github.com/chanzuckerberg/sci-components/issues/577
*/Or something better!
There was a problem hiding this comment.
Great, thanks for the suggestion! I have finally updated to add the comment as suggested!
Specified why I made the change I did and pointed to the GH ticket for context re #578
masoudmanson
added
Released
and removed
Ready for review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Callout component can now exist sans icon by adding icon={false} as a prop
re #577
Summary
Callout
Github issue: #577
Recommended fix (per @tihuan ), documenting here for posterity:
Change the source code from
if (icon) return icon;to:
if (icon !== undefined) return icon;"And then the following will work:
icon={false} - it will not take up space in the component. Similar to display: none;
icon={<></>} - it will still take up space in the component, but just empty space. Similar to visibility: hidden;
icon={null} - this is a little counter intuitive, but it will fall back to using MUI's iconMapping"
Checklist
defaultTheme.tsused wherever possibleNote that I am also having David Ruiz help with reviewing bug fixes for SDS Hackday (which fixing this bug was part of), but I couldn't find him listed yet when I went to add reviewers