Skip to content
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

Restore beta label for the desktop editor settings #7775

Merged
merged 1 commit into from
Jan 23, 2022

Conversation

gtsiolis
Copy link
Contributor

@gtsiolis gtsiolis commented Jan 22, 2022

Description

Following the changes in #7653, this will restore beta label for the desktop editor settings. Cc @mustard-mh

See also relevant discussion (internal).

How to test

  1. Go to /preferences
  2. Notice there's a beta label next to Desktop Editor settings.

Screenshots

BEFORE AFTER
settings-before settings-after

Release Notes

NONE

@roboquat roboquat added release-note-none team: webapp Issue belongs to the WebApp team size/XS labels Jan 22, 2022
@codecov
Copy link

codecov bot commented Jan 22, 2022

Codecov Report

Merging #7775 (ab448a0) into main (c3bcf63) will decrease coverage by 1.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7775      +/-   ##
==========================================
- Coverage   11.54%   10.28%   -1.26%     
==========================================
  Files          20       18       -2     
  Lines        1169     1001     -168     
==========================================
- Hits          135      103      -32     
+ Misses       1031      897     -134     
+ Partials        3        1       -2     
Flag Coverage Δ
components-gitpod-cli-app 10.28% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3bcf63...ab448a0. Read the comment docs.

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: d92f52cd6dbdfecdfb62da53f36c94d1047a34f1

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Also looks good to me, many thanks for fixing this!

See quick note about the ineffective self-center flex class FYI.

/approve

@@ -124,7 +124,7 @@ export default function Preferences() {
}
</>}
{desktopIdeOptions && <>
<h3 className="mt-12">Desktop Editor</h3>
<h3 className="mt-12">Desktop Editor <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Beta</PillLabel></h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: self-center is a flex related class (stands for align-self: center). It has no effect here, because the parent <h3> is not a flex container (i.e. it doesn't have the flex class, so it's using the default layout, where align-self doesn't exist / has no effect).

I think that's why the label is a little low (vertically). To fix it, you could make the <h3> a flex (also, it's generally preferable to center all the items in the flex with a items-center class on the parent, as opposed to centering each item separately using self-center on the children):

Suggested change
<h3 className="mt-12">Desktop Editor <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Beta</PillLabel></h3>
<h3 className="mt-12 flex items-center">Desktop Editor <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2">Beta</PillLabel></h3>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insights, @jankeromnes! FWIW, I blindly copied the label component a couple of lines below but definitely worth fixing this alignement.

<h3 className="mt-12">Dotfiles <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Beta</PillLabel></h3>

I think that's why the label is a little low (vertically).

Thanks for noticing this! Makes me feel a little less crazy. 🧡

@jankeromnes
Copy link
Contributor

/approve no-issue

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iQQBot, jankeromnes

Associated issue requirement bypassed by: jankeromnes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit f3300f0 into main Jan 23, 2022
@roboquat roboquat deleted the gt/restore-beta-label branch January 23, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants