feat(site): refactor template version editor layout #10912

BrunoQuaresma · 2023-11-28T00:10:43Z

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

code-asher

The new layout looks great! Everything works perfectly and the code looks good too.

site/src/AppRouter.tsx

code-asher · 2023-11-28T18:13:58Z

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 · 2023-11-28T19:32:29Z

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>

Parkreiner

Looks good! Just had a comment about the auto-scroll effect logic

Parkreiner · 2023-11-28T18:37:07Z

site/src/components/WorkspaceBuildLogs/Logs.tsx

@@ -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`}>


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

Parkreiner · 2023-11-28T18:40:17Z

site/src/pages/TemplateVersionEditorPage/MonacoEditor.tsx

@@ -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(() => {


Just curious: do you know why we needed the resize listeners originally?

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

Parkreiner · 2023-11-28T19:21:40Z

site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx

+    if (buildLogsRef.current) {
+      buildLogsRef.current.scrollTop = buildLogsRef.current.scrollHeight;
+    }
+  }, [buildLogs]);


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 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

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:

I think swapping in useLayoutEffect would minimize the risk of UI flickers from the scroll happening

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?

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?

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.

Parkreiner · 2023-11-28T19:25:25Z

site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx

+            height: 48,
+            borderBottom: `1px solid ${theme.palette.divider}`,
+            display: "grid",
+            gridTemplateColumns: "1fr 2fr 1fr",


How does this scale as the screen gets more narrow?

Quite well actually.

Parkreiner · 2023-11-28T19:27:20Z

site/src/pages/TemplateVersionEditorPage/TemplateVersionEditor.tsx

+          css={{
+            display: "flex",
+            flex: 1,
+            flexBasis: 0,


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

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 😢

BrunoQuaresma added 7 commits November 27, 2023 23:13

Add base template editor behavior

90c68a3

Finish details

5c15516

Fix test

ec90ce1

Minor fixes

b2f0054

Allow submit publish form using keyboard

3c4c0f5

Add link to the header

2735d25

Add better loading

bbe7894

BrunoQuaresma requested a review from a team November 28, 2023 00:10

BrunoQuaresma self-assigned this Nov 28, 2023

BrunoQuaresma requested review from code-asher and removed request for a team November 28, 2023 00:10

Fix fmt

9ed1474

code-asher approved these changes Nov 28, 2023

View reviewed changes

site/src/AppRouter.tsx Outdated Show resolved Hide resolved

Update site/src/AppRouter.tsx

46a1944

Co-authored-by: Asher <ash@coder.com>

BrunoQuaresma changed the title ~~feat(site): new editor layout~~ feat(site): refactor template version editor layout Nov 28, 2023

BrunoQuaresma merged commit e9c12c3 into main Nov 28, 2023

BrunoQuaresma deleted the bq/new-editor branch November 28, 2023 19:42

github-actions bot locked and limited conversation to collaborators Nov 28, 2023

Parkreiner approved these changes Nov 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(site): refactor template version editor layout #10912

feat(site): refactor template version editor layout #10912

Uh oh!

BrunoQuaresma commented Nov 28, 2023 •

edited

Loading

Uh oh!

code-asher left a comment

Uh oh!

Uh oh!

code-asher commented Nov 28, 2023 •

edited

Loading

Uh oh!

BrunoQuaresma commented Nov 28, 2023

Uh oh!

Parkreiner left a comment

Uh oh!

Parkreiner Nov 28, 2023

Uh oh!

Parkreiner Nov 28, 2023

Uh oh!

BrunoQuaresma Nov 29, 2023

Uh oh!

Parkreiner Nov 28, 2023

Uh oh!

BrunoQuaresma Nov 29, 2023

Uh oh!

Parkreiner Nov 28, 2023

Uh oh!

BrunoQuaresma Nov 29, 2023

Uh oh!

Parkreiner Nov 28, 2023

Uh oh!

BrunoQuaresma Nov 29, 2023

Uh oh!

Uh oh!

feat(site): refactor template version editor layout #10912

feat(site): refactor template version editor layout #10912

Uh oh!

Conversation

BrunoQuaresma commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

code-asher commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrunoQuaresma commented Nov 28, 2023

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrunoQuaresma commented Nov 28, 2023 •

edited

Loading

code-asher commented Nov 28, 2023 •

edited

Loading