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
[CRITICAL] Implement <WorkspaceWorkflowsApprovalsCreatePage /> for Workflow Creation #46798
[CRITICAL] Implement <WorkspaceWorkflowsApprovalsCreatePage /> for Workflow Creation #46798
Changes from 17 commits
88e2519
b0878c5
66c99d3
a0c289b
11372db
d243bd5
d1f0dce
5108e12
062e45a
c0734c8
89f6ee0
8405648
3a39deb
c4da7e3
dbb4311
e92b654
d4ae460
77555f3
1c6ae4b
31d8863
faee4e3
ebd1e05
9fa7889
69c31e2
b67e8b8
ba8a3d3
755954c
603b6ff
230080e
54453fb
b9b7937
68d5caf
64e7f0b
cd36817
436457b
4c0078a
17cab91
f554da6
6707500
c38d5d7
d74ed36
f9ea365
9bde486
e732c6c
05c552f
914bb1f
30d8a6a
14f6eb2
69ab537
5f3f64f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Rename to
getHTMLMessage
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.
Renamed to
HTMLMessage
(as this is not a function but a memo)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.
Please rename these to
shouldRenderHintAsHTML
andshouldRenderErrorAsHTML
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 are other props in this component that are named similary, are we sure we want to name it differently?
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.
Yes, I think having them more explicit is better than consistency. In fact, I'd suggest renaming all of them in order to really clean it up.
As an external consumer of the component, I wouldn't really have any idea what "parse text" implies and I'd have to dig into the component and look at the code to try to understand it.
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.
but to be clear, I'm not asking you to rename the other props in this PR :D Maybe you or I could do a followup PR to clean those up 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.
I'll do it in a separate PR 👍
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.
@blazejkustra Do you confirm this translation with the internal team before?
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.
Yes, here's a doc