[student-libraries] Exporter success page #31487
Conversation
| copyChannelId = () => { | ||
| let channelId = document.getElementById('library-sharing'); | ||
| channelId.select(); | ||
| channelId.setSelectionRange(0, 999); |
There was a problem hiding this comment.
Can you add a comment explaining these bounds?
There was a problem hiding this comment.
I realized that since we are supplying the channel-id, we don't have to worry about unexpected large values, so I just deleted the line.
| </form> | ||
| </div> | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
nit: since this is an exhaustive match on an enum value, consider a switch statement instead- that way if we add a fourth state we would immediately notice, rather than it silently falling into this else.
| </Heading2> | ||
| <div> | ||
| <p style={{fontSize: 20}}>{i18n.libraryPublishExplanation()}</p> | ||
| <div style={{display: 'flex', justifyContent: 'center'}}> |
There was a problem hiding this comment.
Do you think it would be cleaner to have the styles specified in a static object rather than inline on each element?
|
Can you prepend [student-libraries] to the title? |
| this.props.onClose(); | ||
| }; | ||
|
|
||
| copyChannelId = () => { | ||
| let channelId = document.getElementById('library-sharing'); |
There was a problem hiding this comment.
Can you use state rather than searching the dom for the element?
There was a problem hiding this comment.
This seemed a like a good enough solution. clipboard isn't as supported in browsers as I would like for something like this and I wasn't sure if it was worth bringing in an additional library. Happy to discuss further.
There was a problem hiding this comment.
Might be worth a quick chat - I'm curious about the clipboard :) Shouldn't block this PR though.
| return ( | ||
| <div style={styles.centerSpinner}> | ||
| <Spinner /> | ||
| </div> | ||
| ); | ||
| } else if (this.state.loadingState === LoadingState.DONE_LOADING) { |
There was a problem hiding this comment.
Given how spread out these if/elseif/else statements are, would it make sense to move the contents into their own functions?
| id="library-sharing" | ||
| onClick={select} | ||
| readOnly="true" | ||
| value={dashboard.project.getCurrentId()} |
There was a problem hiding this comment.
NIT: Rather than getting the id from dashboard.project, it might be better to get it from the libraryClient response. That way, you know for sure you have the right project ID.
There was a problem hiding this comment.
AFAICT, it wouldn't be possible for the two to get out of sync?
| onClick={select} | ||
| readOnly="true" | ||
| value={dashboard.project.getCurrentId()} | ||
| style={{cursor: 'copy', width: 300}} |
There was a problem hiding this comment.
Can you add height: 25px too? I noticed the box and the button are different heights :)
…dot-org into exporter-success-page
epeach
changed the title
Description
Add a success page after a library is published. Displays channel id and a button to copy the channel id. (Note: the text has been changed from the screenshot below to display ID instead of code)
Testing story
Updated the tests to check that channel id is present.
Reviewer Checklist: