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.
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
[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025
[SharedUX] Migrate PageTemplate > NoDataPage > NoDataCard #127025
Changes from 4 commits
94a2b3e
1e197b9
f3eae30
bec6e63
24bcf27
112e6d9
71b8fc7
346ec1f
6e3679b
1ef414c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why separate the component type into a separate file?
Also since it's the direct props of NoDataCard, let's name it consistently:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this type will be reused for
ElasticAgentCard
once I migrate it over, so that component can read fromtypes
file as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to be changing prop names at this time, as I don't know where down the line in PageTemplate is this component used, and renaming them all will add additional work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchaos How does EUI handle this kind of flexibility? Is there a pattern?
@majagrubic Be sure this is the TS type you want to use.
https://dev.to/fromaline/jsxelement-vs-reactelement-vs-reactnode-2mh2
https://github.com/typescript-cheatsheets/react#useful-react-prop-type-examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there's a specific pattern, but the idea here is that the consumer can supply just the button label (string) or replace the whole button their own (in case they don't want to use the same medium, filled button). I guess technically
string
is included inReactNode
, so maybe that's duplicative. But I think the explicitness here is helpful because an element would replace the whole default EuiButton where the string is just the label. Up to you all if you think there's a better pattern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I was thinking maybe
string | EuiButtonProps
.Then we'd have some control over precisely what component we rendered there, and could force a
fill
orcolor
to keep the usage consistent, now or in the future.It's probably minor, but I'm one who leans toward keeping the contract strict at the start, and then expanding outward as necessary, to avoid large-scale refactors later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do that. But specifically EuiButton doesn't have a
label
prop, it's label is just thechildren
, so you'd likely need to append this prop type or rely on consumers to passchildren
as a key.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I think the problem with that, that I'm now remembering, is that you can't go from say an EuiButton to EuiEmptyButton by props alone since they're completely different components. So an example is if the footer button is actually not a CTA we want to promote, or say we need to provide 2 actions instead of just one, there's no way to remove the default button without replacing it completely with a
ReactNode
.