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.

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

scm: fixed focused border of scm textarea #7340

Merged
merged 1 commit into from
Mar 16, 2020
Merged

scm: fixed focused border of scm textarea #7340

merged 1 commit into from
Mar 16, 2020

Conversation

vince-fugnitto
Copy link
Member

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

What it does

Fixes #7338

The following commit fixes the issue where the scm textarea does not display a focused state.
Note: The change also updates the default border to the issue #3544 is not re-introduced.

How to test

  1. start the application and open the scm widget
  2. verify that when selecting the textarea (message), the focused border is correctly displayed

aaa

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added ui/ux issues related to user interface / user experience scm issues related to the source control manager labels Mar 14, 2020
@vince-fugnitto vince-fugnitto self-assigned this Mar 14, 2020
Anasshahidd21
Anasshahidd21 approved these changes Mar 15, 2020
View reviewed changes
Copy link
Contributor

@Anasshahidd21 Anasshahidd21 left a comment

Choose a reason for hiding this comment

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

Works fine! Tested with browser, the git message bar has the desired border on focus.

@kittaakos
Copy link
Contributor

Thank you for the quick solution, @vince-fugnitto 👍

From the changelog:

Theia comes though with predefined css class names: theia-input, theia-select and theia-button to style input/textarea, select and button elements correspondingly. Existing components were refactored to use them.

.theia-input:

theia/packages/core/src/browser/style/index.css

Lines 55 to 63 in 9457f80

.theia-input {
background: var(--theia-input-background);
color: var(--theia-input-foreground);
border: var(--theia-border-width) solid var(--theia-input-border);
font-family: var(--theia-ui-font-family);
font-size: var(--theia-ui-font-size1);
line-height: var(--theia-content-line-height);
padding-left: 5px;
}

The commit message textArea has the theia-input class (on my branch):

<textarea class="theia-scm-input-message theia-input theia-scm-input-message-idle" id="theia-scm-input-message" placeholder="Message (press Cmd Enter to commit)" tabindex="1" rows="1" style="max-height: 132px; overflow: hidden; overflow-wrap: break-word; height: 30px;">inde</textarea>

Does anyone know why do we need to treat the SCM textArea differently?

akosyakov
akosyakov reviewed Mar 16, 2020
View reviewed changes
packages/scm/src/browser/style/index.css
@@ -102,7 +102,11 @@
}

.theia-scm-input-message-container textarea:not(:focus) {
border: none;
border: var(--theia-border-width) solid var(--theia-input-background);
Copy link
Member

@akosyakov akosyakov Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

background color for border color? We should use semantically meaningful colors. Are you sure that there is a bug at all. In VS Code background is used for contrast, not border. Borders only for some elements or in HC theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that there is a bug at all.

Yes: #7338 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

ok, but it should be --theia-input-border color

Please see https://code.visualstudio.com/api/references/theme-color#input-control

input.border: Input box border.

Copy link
Member

@akosyakov akosyakov Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Also you can have a look at reference implementation from VS
Code: https://github.com/microsoft/vscode/blob/89906c073a8fc8e60f344c5623f5c73adcf039f1/src/vs/workbench/contrib/scm/browser/repositoryPane.ts#L1108-L1112

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but it should be --theia-input-border color

Well, ideally it should be nothing. The theia-input CSS should work out of the box.

Copy link
Member

@akosyakov akosyakov Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

☝️ it is reproducible with HC theme, look at how it looks in VS Code and Theia without focus

VS Code:
Screenshot 2020-03-16 at 16 01 04

Theia:
Screenshot 2020-03-16 at 16 01 09

But the bug on master as well, please file an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been working on fixing input validation:

Screen Shot 2020-03-16 at 12 36 45 PM

This is with the correct theming applied.

Copy link
Member

@akosyakov akosyakov Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@vince-fugnitto It is not about validation without any errors and focus border should be shown in HC theme as in VS Code. It cannot work if we use background color for border color.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vince-fugnitto It is not about validation without any errors and focus border should be shown in HC theme as in VS Code. It cannot work if we use background color for border color.

I have a pull-request handling the theming for validation: #7351
Would you like me to handle the hc theme as well for inputs? I understand that hc theming was omitted as part of the first iteration of theming support #6743

Copy link
Member

Choose a reason for hiding this comment

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

HC theme is a way to reproduce the issue, it does not work for any theme which defines different input.border.

@kittaakos
Copy link
Contributor

@vince-fugnitto, your changes seem to be incomplete.

See the textArea border when there is an error,

Screen Shot 2020-03-16 at 14 13 04

and when there is a warning.
Screen Shot 2020-03-16 at 14 12 50

For the latter case, I would expect to have an orangeish color for the border; instead, it is the default border color.

@vince-fugnitto
scm: fixed focused border of scm textarea
9e85a58 
Fixes #7338

The following commit fixes the issue where the `scm`
textarea does not display a focused state.

The change also updates the default border to the issue
#3544 is not re-introduced.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto
Copy link
Member Author

@vince-fugnitto, your changes seem to be incomplete.

Thank you @kittaakos, I've updated the code:

Screen Shot 2020-03-16 at 10 00 55 AM

@akosyakov akosyakov added theming issues related to theming and removed ui/ux issues related to user interface / user experience labels Mar 16, 2020
kittaakos
kittaakos approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

👍 Thank you!

screencast 2020-03-16 15-35-19

@vince-fugnitto vince-fugnitto merged commit b553a22 into master Mar 16, 2020
@vince-fugnitto vince-fugnitto deleted the vf/GH-7338 branch March 16, 2020 16:41
kittaakos pushed a commit that referenced this pull request May 18, 2020
GH-7834: Restored the find widget's "border" color.
5a6833e 
`--theia-input-border` as the `outline-color` had no effect:
#7340 (comment)

Fixes #7834

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@kittaakos kittaakos mentioned this pull request May 18, 2020
Merged
1 task
kittaakos pushed a commit that referenced this pull request May 19, 2020
@kittaakos
GH-7834: Restored the find widget's "border" color.
46255a2 
`--theia-input-border` as the `outline-color` had no effect:
#7340 (comment)

Fixes #7834

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov left review comments

@kittaakos kittaakos kittaakos approved these changes

@Anasshahidd21 Anasshahidd21 Anasshahidd21 approved these changes

Assignees

@vince-fugnitto vince-fugnitto

Labels
scm issues related to the source control manager theming issues related to theming
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[git][ui/ux] no border around the git commit message textarea when element has focus
4 participants
@vince-fugnitto @kittaakos @akosyakov @Anasshahidd21