Skip to content

feat: Add template settings page #3557

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 8 commits into from
Aug 18, 2022
Merged

feat: Add template settings page #3557

merged 8 commits into from
Aug 18, 2022

Conversation

BrunoQuaresma
Copy link
Collaborator

Related to #2221

Demo:

Screen.Recording.2022-08-18.at.14.40.46.mov

@BrunoQuaresma BrunoQuaresma self-assigned this Aug 18, 2022
@BrunoQuaresma BrunoQuaresma requested a review from a team as a code owner August 18, 2022 17:43
@BrunoQuaresma BrunoQuaresma changed the title Add settings page feat: Add template settings page Aug 18, 2022
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the full-page modal design will bite us in the relatively short-term future. When we add more options, the page will only get longer and eventually have to be recreated (probably even if we added 4-5 more).

image

I like the hierarchy of design here from Vercel. It displays the resource being edited (in this case our coder.com deployment) and has a scalable design for adding more options. The design in this PR is functional for now but doesn't have the professional polish I think would be nice. That is nice to have though, so I certainly won't block this change!

(I don't have time to do a code review right now, I just watched the demo, so someone else should review!)

Copy link
Contributor

@presleyp presleyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good and chromatic mostly looks good - I didn't see any error stuff on the save error message story so I denied it in Chromatic just so you can take a look.

@BrunoQuaresma
Copy link
Collaborator Author

The code looks good and chromatic mostly looks good - I didn't see any error stuff on the save error message story so I denied it in Chromatic just so you can take a look.

Yes, it is because you have to click outside the form... Idk why this happens, but it looks like MUI doesn't display the error if it is focused - the input is auto-focus. Not sure if I should add some auto-focus logic to enable it only when the form doesn't have errors... sounds too much. What do you think @presleyp ?

Co-authored-by: Presley Pizzo <1290996+presleyp@users.noreply.github.com>
@BrunoQuaresma
Copy link
Collaborator Author

@kylecarbs I agree with you. I just would expect we have more options because a page like the Vercel one, only looks good if you have options enough to fill the space.

@BrunoQuaresma
Copy link
Collaborator Author

@presleyp I find out I can use initialTouch to make it work.

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

Successfully merging this pull request may close these issues.

3 participants