-
Notifications
You must be signed in to change notification settings - Fork 937
feat(site): refactor template version editor layout #10912
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
Conversation
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.
The new layout looks great! Everything works perfectly and the code looks good too.
One question: when you build the template it changes the name at the top but not the URL, is that intentional? It does change it after you press publish. |
Yes but no strong reason tho. I just thought "publishing" would be a good "checkpoint" to save the URL. |
Co-authored-by: Asher <ash@coder.com>
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.
Looks good! Just had a comment about the auto-scroll effect logic
@@ -24,10 +24,14 @@ export const Logs: FC<React.PropsWithChildren<LogsProps>> = ({ | |||
className = "", | |||
}) => { | |||
return ( | |||
<div css={styles.root} className={className}> | |||
<div css={styles.root} className={`${className} logs-container`}> |
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 forgot the details for when something can and can't support the css
, but I'm thinking that this could be consolidated into a single css
prop
@@ -13,22 +12,6 @@ export const MonacoEditor: FC<{ | |||
onChange?: (value: string) => void; | |||
}> = ({ onChange, value, path }) => { | |||
const theme = useTheme(); | |||
const [editor, setEditor] = useState<editor.IStandaloneCodeEditor>(); | |||
useLayoutEffect(() => { |
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.
Just curious: do you know why we needed the resize listeners originally?
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.
To resize the editor when the bottom panel was open/closed but Idk why we were using it since the autoLayout prop was set to true 🤔 maybe someone faced a bug and used the listener? I tested both cases and they worked without it tho
if (buildLogsRef.current) { | ||
buildLogsRef.current.scrollTop = buildLogsRef.current.scrollHeight; | ||
} | ||
}, [buildLogs]); |
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.
Trying to wrap my head around what's expected here
If I'm understanding things right:
- Whenever
buildLogs
changes, the effect triggers, scrolling the user to the bottom of the containerscrollTop
measures the distance from the top of the container to the top of the visible content- By setting it to the
scrollHeight
, which is the total height of the overflow content, the effect always scrolls the user to the bottom of the container - I think (and I do mean think) the value will always overshoot, because the value we want would be
scrollHeight
minus the height of the container, but I'm guessing browsers automatically round down to avoid weird edge cases
- The value of
buildLogs
comes further up from auseQuery
call, and because of that, it should always beundefined
on the initial render- There is the check for the data being defined in the top-level page component, but the data can still be undefined in the view component
Things that come to mind:
- I think swapping in
useLayoutEffect
would minimize the risk of UI flickers from the scroll happening - Since
buildLogs
can beundefined
on the initial render, do we want the scroll to always happen on mount, or should the condition for scrolling becomebuildLogsRef.current && buildLogs !== undefined
? - How is
buildLogs
able to be updated over time?- It seems that it's websocket-based, and it will keep having logs appended to an array?
- In that case, I'm worried about the case where the user scrolls up to read their logs, but then a new log comes in, and because of the new array reference, the effect violently pushes the user back to the bottom of the container
- Should the effect be updated to check if the user was already at the bottom of the content on the previous render before triggering the scroll?
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.
1 - No need to use useLayoutEffect
for now since the panel only shows when the build happens. Even without anything, the scroll would behave the same. At least on my tests, I didn't see anything strange.
2 - No problem with the scroll happening when it is undefined, there is no side effect on that.
3 - I think it is a good one but I see this as an "upgrade" of the feature. If we see users asking for that, we could definitely implement it.
height: 48, | ||
borderBottom: `1px solid ${theme.palette.divider}`, | ||
display: "grid", | ||
gridTemplateColumns: "1fr 2fr 1fr", |
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.
How does this scale as the screen gets more narrow?
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.
Quite well actually.
css={{ | ||
display: "flex", | ||
flex: 1, | ||
flexBasis: 0, |
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.
Doesn't flexBasis: 0
tell the browser that ideally, the element should have a width of 0 (before accounting for flex stretching)?
Wondering if this was supposed to be flexShrink
or something, but the overflow makes me wonder if this might also be a layout hack
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.
This is a good one. I did that to make this element hit 100% height but I have to say that I don't remember why I did that in the past - I just moved it from the makeStyles - and be sorry for not adding a helpful comment 😢
Demo:
https://github.com/coder/coder/assets/3165839/cb4519b2-b0e8-4f66-8dc8-6d3bc277f97b