-
Notifications
You must be signed in to change notification settings - Fork 352
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
CodeEditor loading property considered by the Editor #11034
Conversation
I would like to see tests for this particular use case not being covered, I will provide as soon as we have an approval about the issue and also the proposal (this PR) |
Preview: https://patternfly-react-pr-11034.surge.sh A11y report: https://patternfly-react-pr-11034-a11y.surge.sh |
@@ -652,6 +653,7 @@ class CodeEditor extends React.Component<CodeEditorProps, CodeEditorState> { | |||
onChange={this.onChange} | |||
onMount={this.editorDidMount} | |||
theme={isDarkTheme ? 'vs-dark' : 'vs-light'} | |||
loading={loading} | |||
{...editorProps} |
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.
editorProps
should already allow loading
to be passed. If we want loading
to be a distinct property, we will need to add it to the props table.
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 missed that this already was in the props table and is unused.
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.
Ah I do see we have this on our props table already, it wasn't being passed.
Using editorProps
would also work, but I'm in favor of adding this method too.
one more additional approver required, please any kind soul aroung? 🙏 😉 |
@Ginxo if you need this in v5, can you please change the base branch to the |
a new PR for V5 already open |
@Ginxo We are still in code freeze right now. We will merge your PR once our release is live. |
What: Closes #11033
Additional issues: