[student-libraries] Refactor the library creation dialog in preparation for adding 'unpublish' functionality #32792
Conversation
jmkulwik
changed the title
| } | ||
| return ( | ||
| <div> | ||
| <p style={styles.alert}>{errorMessage}</p> |
There was a problem hiding this comment.
It looks like this will be rendered even when there is not error message?
| libraryDescription | ||
| ); | ||
|
|
||
| // TODO: Display final version of error and success messages to the user. |
There was a problem hiding this comment.
Is there anything left to do related to this TODO?
| class LibraryCreationDialog extends React.Component { | ||
| static propTypes = { | ||
| libraryClientApi: PropTypes.object.isRequired, | ||
|
|
||
| // From Redux |
| return <div>{errorMessage}</div>; | ||
| }; | ||
| render() { | ||
| const {message} = this.props; |
There was a problem hiding this comment.
Nit: do these need to be two different lines?
There was a problem hiding this comment.
No, they don't - It seems like it's becoming good practice to do it this way (although certainly not necessary in this case).
Couple examples of this pattern:
https://towardsdatascience.com/react-best-practices-804def6d5215
https://brewhouse.io/blog/2015/03/24/best-practices-for-component-state-in-reactjs.html
|
unit tests passed: https://drone.cdn-code.org/code-dot-org/code-dot-org/9416/ |
| bodyContent = <ErrorDisplay message={i18n.libraryNoFunctionsError()} />; | ||
| break; | ||
| default: | ||
| bodyContent = this.displayContent(); |
There was a problem hiding this comment.
It seems kind of weird to be using the default case as a legitimate/common case. Would it make sense to add a DialogState to cover this case (so that the options of DialogState actually exhaustively cover the cases, and the default case of this switch statement would catch if we like, add a new DialogState or something like that?)
|
|
||
| displayLoadingState = () => { | ||
| export class LoadingDisplay extends React.Component { |
There was a problem hiding this comment.
Could some of these be pure functional components instead of classes? https://towardsdatascience.com/react-best-practices-804def6d5215
This splits the LibraryCreationDialog into three parts
Background
Unpublish will add significant functionality to the library creation dialog. It will allow users to unpublish their library. It will also allow us to detect if a library has already been created, and, if it has, to merge the existing metadata into the library.
Testing story
Updated old tests
Reviewer Checklist: