Skip to content

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

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma commented Nov 28, 2023

  • New layout
  • Fix version name not being updated when published
  • Fix published success message not being displayed when previously missed
  • Fix the publish version modal not being submitted on the keyboard enter
  • Improve initial loading state
  • Update URL when a version is published
  • Auto-scroll logs

Demo:
https://github.com/coder/coder/assets/3165839/cb4519b2-b0e8-4f66-8dc8-6d3bc277f97b

@BrunoQuaresma BrunoQuaresma requested a review from a team November 28, 2023 00:10
@BrunoQuaresma BrunoQuaresma self-assigned this Nov 28, 2023
@BrunoQuaresma BrunoQuaresma requested review from code-asher and removed request for a team November 28, 2023 00:10
Copy link
Member

@code-asher code-asher left a 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.

@code-asher
Copy link
Member

code-asher commented Nov 28, 2023

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.

@BrunoQuaresma
Copy link
Collaborator Author

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>
@BrunoQuaresma BrunoQuaresma changed the title feat(site): new editor layout feat(site): refactor template version editor layout Nov 28, 2023
@BrunoQuaresma BrunoQuaresma merged commit e9c12c3 into main Nov 28, 2023
@BrunoQuaresma BrunoQuaresma deleted the bq/new-editor branch November 28, 2023 19:42
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2023
Copy link
Member

@Parkreiner Parkreiner left a 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`}>
Copy link
Member

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(() => {
Copy link
Member

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?

Copy link
Collaborator Author

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]);
Copy link
Member

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:

  1. Whenever buildLogs changes, the effect triggers, scrolling the user to the bottom of the container
    • scrollTop 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
  2. The value of buildLogs comes further up from a useQuery call, and because of that, it should always be undefined 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:

  1. I think swapping in useLayoutEffect would minimize the risk of UI flickers from the scroll happening
  2. Since buildLogs can be undefined on the initial render, do we want the scroll to always happen on mount, or should the condition for scrolling become buildLogsRef.current && buildLogs !== undefined?
  3. 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?

Copy link
Collaborator Author

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",
Copy link
Member

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?

Copy link
Collaborator Author

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,
Copy link
Member

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

Copy link
Collaborator Author

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 😢

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants