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.

Sign up for GitHub

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

Jump to bottom

[dashboard] Env Variables & Misc Fixes #3581

Merged

svenefftinge merged 6 commits into main from se/misc_dashboard

Mar 26, 2021

Member

svenefftinge commented Mar 25, 2021 •

edited

Loading

This PR fixes #3400 as well as
the following items from #3507

Avatar Menu actions don't work (settings / logout)
Pressing ESC opens up the "new workspace" (at least after it was opened once)
Persist top menu state on Workspaces and Settings
(https://gitpod.slack.com/archives/C01KGM9BUNS/p1615919354035200).
When clicking on Workspaces the top menu always move to Active workspaces.
Can we also remove the responsive layout of the top menu and profile menu?

Member Author

svenefftinge commented Mar 25, 2021 •

edited by werft-gitpod-dev-com bot

Loading

/werft run

👍 started the job as gitpod-build-se-misc-dashboard.6

Member Author

svenefftinge commented Mar 25, 2021 •

edited by werft-gitpod-dev-com bot

Loading

/werft run

👍 started the job as gitpod-build-se-misc-dashboard.7

Member Author

svenefftinge commented Mar 25, 2021 •

edited by werft-gitpod-dev-com bot

Loading

/werft run

👍 started the job as gitpod-build-se-misc-dashboard.9

svenefftinge force-pushed the se/misc_dashboard branch 2 times, most recently from 2ef497d to 68ecbe0 Compare

March 25, 2021 12:23

svenefftinge marked this pull request as ready for review

March 25, 2021 12:30

svenefftinge changed the title ~~[dashboard] Misc Improvements & Fixes~~ [dashboard] Env Variables & Misc Fixes

svenefftinge requested a review from gtsiolis

March 25, 2021 12:32

svenefftinge force-pushed the se/misc_dashboard branch from 68ecbe0 to 31742b3 Compare

March 25, 2021 12:33

Contributor

gtsiolis commented Mar 25, 2021 •

edited by werft-gitpod-dev-com bot

Loading

Looking at this now! 👀

gtsiolis approved these changes

View reviewed changes

Contributor

gtsiolis left a comment

C'est magnifique! 🌟 Thanks @svenefftinge! Added a few minor comments and one for adding a confirmation modal I didn't include in the initial specs. 🏀

components/dashboard/src/settings/EnvironmentVariables.tsx Outdated Show resolved Hide resolved

components/dashboard/src/settings/EnvironmentVariables.tsx Outdated

+                      {envVars.length === 0
+                          ? <div className="bg-gray-100 rounded-xl w-full h-96">
+                              <div className="pt-28 flex flex-col items-center w-96 m-auto">
+                                  <h3 className="text-center pb-3">No Environment Variables</h3>

Contributor

gtsiolis Mar 25, 2021

issue: Minopr heading color.

Suggested change

      
                                <h3 className="text-center pb-3">No Environment Variables</h3>
          
                                <h3 className="text-center pb-3 text-gray-500">No Environment Variables</h3>

Member Author

svenefftinge Mar 26, 2021

FWIW The heading color on the empty workspaces list page is not like that. It has a different background though, but I'd argue that a gray background would make it even less attractive to also have a grayish font color?

Contributor

gtsiolis Mar 26, 2021 •

edited

Loading

FWIW The heading color on the empty workspaces list page is not like that.

Yes, we could ~~definitely~~ change that, too. Also, it doesn't have to be too much gray, only a couple of tones lower than the page headings. I'll open a PR for such small fixes today or later.

I'd argue that a gray background would make it even less attractive to also have a grayish font color?

Fair point. No strong feelings on this one.

components/dashboard/src/settings/EnvironmentVariables.tsx

+                          </div> : null}
+                          <div className="mt-4">
+                              <h4>Name</h4>
+                              <input className="w-full" type="text" value={ev.name} onChange={(v) => { update({name: v.target.value}) }} />

Contributor

gtsiolis Mar 25, 2021

issue: Minor input style issue.

Suggested change

      
                            <input className="w-full" type="text" value={ev.name} onChange={(v) => { update({name: v.target.value}) }} />
          
                            <input className="w-full border-2 border-gray-400" type="text" value={ev.name} onChange={(v) => { update({name: v.target.value}) }} />

components/dashboard/src/settings/EnvironmentVariables.tsx

+                          </div>
+                          <div className="mt-4">
+                              <h4>Value</h4>
+                              <input className="w-full" type="text" value={ev.value} onChange={(v) => { update({value: v.target.value}) }} />

Contributor

gtsiolis Mar 25, 2021

issue: Minor input style issue.

Suggested change

      
                            <input className="w-full" type="text" value={ev.value} onChange={(v) => { update({value: v.target.value}) }} />
          
                            <input className="w-full border-2 border-gray-400" type="text" value={ev.value} onChange={(v) => { update({value: v.target.value}) }} />

components/dashboard/src/settings/EnvironmentVariables.tsx Outdated Show resolved Hide resolved

components/dashboard/src/settings/EnvironmentVariables.tsx Outdated Show resolved Hide resolved

components/dashboard/src/settings/EnvironmentVariables.tsx Outdated Show resolved Hide resolved

components/dashboard/src/settings/EnvironmentVariables.tsx

+                          return 'Value must not be empty.';
+                      }
+                      if (pattern.trim() === '') {
+                          return 'Scope must not be empty.';

Contributor

gtsiolis Mar 25, 2021

praise: Loved the kumquat colors here! Will get all validations consistent in the next iteration when auditing colors, and more. 🍊

components/dashboard/src/settings/EnvironmentVariables.tsx Outdated

+                                  <button onClick={add} className="font-medium">New Environment Variable</button>
+                              </div>
+                          </div>
+                          : <div>

Contributor

gtsiolis Mar 25, 2021

issue: Spacing missing.

thought: The amount of semantic inconsistencies (HTML structure) between the same elements and components is slightly increasing! ⚠️

Suggested change

      
                        : <div>
          
                        : <div class="space-y-2">

Member Author

svenefftinge Mar 26, 2021

I've applied the change as suggested but wonder if it is not better to apply space on the childs or at least only on one or the other not on both.

Contributor

gtsiolis Mar 26, 2021

Absolutely, it would be better to apply spacing on the child elements as the filter section could be a separate component in the long run (see design system).

The suggestion was due to existing usage[1][2][3][...] in order to keep the same structure (technical debt).

components/dashboard/src/settings/EnvironmentVariables.tsx

+                                                      {
+                                                          title: 'Delete',
+                                                          customFontStyle: 'text-red-600 hover:text-red-800',
+                                                          onClick: () => deleteV(ev)

Contributor

gtsiolis Mar 25, 2021

issue: We should definitely add a confirmation modal here! What do you think?

Title: Delete Variable
Body: Are you sure you want to delete this environment variable?
Actions: Cancel (Secondary), Delete. Variable (Primary)

Draft Prototype

Member Author

svenefftinge Mar 26, 2021

filed #3604

Member

akosyakov commented Mar 25, 2021

It would be good to merger this PR to verify #3569 properly with wildcard repo patterns.

Member Author

svenefftinge commented Mar 26, 2021

It would be good to merger this PR to verify #3569 properly with wildcard repo patterns.

Will work on the feedback and then merge

Member Author

svenefftinge commented Mar 26, 2021 •

edited by werft-gitpod-dev-com bot

Loading

/werft run

👍 started the job as gitpod-build-se-misc-dashboard.13

svenefftinge added 6 commits

March 26, 2021 10:41


          Update README.md

8098e7f


          [dashboard] fixed and simplified modals

58c72ac

and context menus


          [dashboard] fix active menu visualization

4eb2096


          [dashboard] fix avatar menu

45e903e


          [dashboard] remove responsive menu

9c78f87


          [dashboard] env variables

a024842

svenefftinge force-pushed the se/misc_dashboard branch from 31742b3 to a024842 Compare

March 26, 2021 10:41

svenefftinge merged commit e4974b4 into main

svenefftinge deleted the se/misc_dashboard branch

March 26, 2021 12:54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet