web: move share snapshot in to modal, show register token URL #2095
Conversation
| @@ -0,0 +1,14 @@ | |||
| package tft | |||
There was a problem hiding this comment.
Maybe "cloud" instead of "tft"?
| <section className="modal-main"> | ||
| <section className="modal-snapshotUrlWrap"> | ||
| <button onClick={props.handleClose}>close</button> | ||
| <button onClick={() => props.handleSendSnapshot(props.state)}> |
There was a problem hiding this comment.
it seems weird to pass modal the entire state. usually you would bind the snapshot to the function, so that this was just invoked as props.sendSnapshot()
| if (!hasLink) { | ||
| return ( | ||
| <div className={showHideClassName}> | ||
| <section className="modal-main"> |
There was a problem hiding this comment.
this isn't what section is for, see
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section#Usage_notes
Do not use the <section> element as a generic container; this is what <div> is for, especially when the sectioning is only for styling purposes. A rule of thumb is that a section should logically appear in the outline of a document.
| <section className="modal-main"> | ||
| <section className="modal-snapshotUrlWrap"> | ||
| <button onClick={props.handleClose}>close</button> | ||
| <button onClick={() => window.open(props.snapshotUrl)}> |
There was a problem hiding this comment.
can you reduce the duplication here by assigning this button to a variable?
e.g.
let button = <button...>
if (hasLink) {
button = <button ...>
}
| transform: translate(-50%,-50%); | ||
| } | ||
|
|
||
| .display-block { |
There was a problem hiding this comment.
i would usually call this u-displayBlock / u-displayNone
in BEM convention, "display-none" typically means "in the display component, there's a sub-element called block", which is obviously not what you mean here
| } | ||
|
|
||
| .modal-main { | ||
| position:fixed; |
There was a problem hiding this comment.
if you've got a fixed element inside another fixed element, that usually means you're confused and not using it correctly. tho i can clean this up in a follow-up pr
| </section> | ||
| ) | ||
| } | ||
| const renderSnapshotModal = (handleOpenModal: () => void) => { |
There was a problem hiding this comment.
this would make more sense as a method of TopBar? that way it has access to this.props. It creates a sub-element of topbar and doesn't seem generally useful outside it
| ) | ||
| } | ||
|
|
||
| const loggedOutTiltCloudCopy = (registerTokenURL: string) => ( |
There was a problem hiding this comment.
it might make more sense if ShareSnapshotModal was a class, so this could be a method
| <section className="modal-main"> | ||
| <section className="modal-snapshotUrlWrap"> | ||
| <button onClick={props.handleClose}>close</button> | ||
| <button onClick={() => window.open(props.snapshotUrl)}> |
There was a problem hiding this comment.
also, this should really be an <a> with an href
| } | null | ||
| IsSidebarClosed: boolean | ||
| SnapshotLink: string | ||
| ShowSnapshotModal: boolean |
There was a problem hiding this comment.
fine for this PR, but it seems weird that all these fields are capitalized
There was a problem hiding this comment.
They're capitalized because they come from Go and we don't override the JSON serialization.
There was a problem hiding this comment.
this property doesn't come from Go, though, right? or am i misunderstanding?
There was a problem hiding this comment.
Ohh you're right @nicks, good call. I can lowercase it.
jazzdan
changed the title
| } | null | ||
| IsSidebarClosed: boolean | ||
| SnapshotLink: string | ||
| ShowSnapshotModal: boolean |
There was a problem hiding this comment.
this property doesn't come from Go, though, right? or am i misunderstanding?
3 participants
It's pretty ugly, but:
After clicking "Share Snapshot":
Will probably need to rope @nicks in for some styling help 😅