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

Don't cover input field with file preference browse button #12586

Merged
merged 3 commits into from
Jul 3, 2023
Merged

Don't cover input field with file preference browse button #12586

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9dfaddb
Don't cover input field with file preference browse button
w-pearson Jun 1, 2023
6727cec
Remove spacing next to the file preference browse button
w-pearson Jun 1, 2023
c7e0db3
Set the preference file button height instead of using padding
w-pearson Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/preferences/src/browser/style/preference-file.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
}

.theia-settings-container .preference-file-button {
position: absolute;
padding: 2px 9px;
right: 4px;
margin-left: 0px;
padding-bottom: 6px;
padding-top: 5px;
Copy link
Member

@msujew msujew Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've noticed there's a small rendering issue on FireFox (it looks fine on Chrome), where the button is actually 27px in height. Maybe we should just fix it to be something like this instead of playing around with paddings?

Suggested change
padding-bottom: 6px;
padding-top: 5px;
height: 26px;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't seem to get the "Windows Exec" preference to show up when running in Firefox (with yarn browser start); I previously tested with Electron. How are you doing it?

Using exact pixel values seems like it would lead to more problems later (unless the height of the text field is also set in pixels, which it doesn't seem to be). I don't see any obvious way to make it automatically use the exact same amount of vertical space/all available vertical space (height: 100% didn't do it).

Copy link
Member

Choose a reason for hiding this comment

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

I've just modified a preference to use the typeDetails: { isFilepath: true } property. See here. In my case, I've used the window.title preference.

unless the height of the text field is also set in pixels, which it doesn't seem to be

It's not, but it's using line-height which is almost the same for the purpose of size calculation as plainly using height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used calc(var(--theia-content-line-height) + 4px) (which is equivalent to 26px). That seems to give correct results with firefox and electron-chromium.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msujew Is the proposed solution fine with you? If so, we could get this PR merged.

}