SlideTabs improvements #49545
Merged
SlideTabs improvements #49545
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
Adds a model-level validation capability to our validation library.
They will be later required in the SlideTabs component
Add support for tooltips and status icons
bl-nero
added
the
no-changelog
This was referenced Nov 28, 2024
gzdunek
reviewed
Nov 29, 2024
gzdunek
approved these changes
Dec 3, 2024
| // property being undefined and not present at all: undefined props would | ||
| // override the default ones, but we want it them to interfere at all. | ||
| const optionalProps: { color?: string; 'aria-label'?: string } = {}; | ||
| if (color != undefined) { |
There was a problem hiding this comment.
Suggested change
| if (color != undefined) { | |
| if (color !== undefined) { |
There was a problem hiding this comment.
Is it really necessary to be so strict about it? I mean, if somehow color (and ariaLabel below) become null, we're gonna have a bug.
There was a problem hiding this comment.
Oh, well, I guess the type system would protect us here in this case. Changing.
| if (color != undefined) { | ||
| optionalProps.color = color; | ||
| } | ||
| if (ariaLabel != undefined) { |
There was a problem hiding this comment.
Suggested change
| if (ariaLabel != undefined) { | |
| if (ariaLabel !== undefined) { |
ravicious
approved these changes
Dec 3, 2024
Comment on lines
+286
to
+288
| // This is one of these rare cases when there is a difference between | ||
| // property being undefined and not present at all: undefined props would | ||
| // override the default ones, but we want it them to interfere at all. |
| optionalProps['aria-label'] = ariaLabel; | ||
| } | ||
|
|
||
| if (statusKind) { |
There was a problem hiding this comment.
Nit: Use an early return instead?
if (!statusKind) {
return null;
}
// This is one of these rare cases when there is a difference between
// property being undefined and not present at all: undefined props would
// override the default ones, but we want it them to interfere at all.
const optionalProps: { color?: string; 'aria-label'?: string } = {};
if (color != undefined) {
optionalProps.color = color;
}
if (ariaLabel != undefined) {
optionalProps['aria-label'] = ariaLabel;
}
return (
<StatusIcon
kind={statusKind}
size={size}
style={{ transition: 'color 0.2s ease-in 0s' }}
{...optionalProps}
/>
);
bl-nero
added a commit
that referenced
this pull request
Dec 6, 2024
* Validation Adds a model-level validation capability to our validation library. * Move tooltips to the design package They will be later required in the SlideTabs component * SlideTabs improvements Add support for tooltips and status icons * Review * Also, rename the tooltip directory * Fix an import * Review: status-related props, extract StatusIcon * Also, rename the tooltip component * review * review * Never return undefined from useValidation()
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 9, 2024
* Validation Adds a model-level validation capability to our validation library. * Move tooltips to the design package They will be later required in the SlideTabs component * SlideTabs improvements Add support for tooltips and status icons * Review * Also, rename the tooltip directory * Fix an import * Review: status-related props, extract StatusIcon * Also, rename the tooltip component * review * review * Never return undefined from useValidation()
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.
Add support for tooltips and status icons
Demo. Note that it demonstrates these features as they are used in a follow-up PR, based on this one.
Requires #49544 (not on its own, but because it's a part of a larger PR train)
Followed up by #49546
Contributes to #46612