-
-
Notifications
You must be signed in to change notification settings - Fork 603
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
Preferences window design refinements #5162
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'm going to trust you with the CSS changes :D
@@ -19,11 +19,11 @@ | |||
<template #view1> | |||
<TextControl | |||
v-model="query" | |||
v-bind:placeholder="'Find…'" | |||
v-bind:placeholder="'Search'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this needs to be a variable so that it becomes translatable. How I did it in all Vue components is like so:
- Define a variable in the script part of the file:
const queryPlaceholderLabel = trans('Search')
- Reference that like so:
v-bind:placeholder="queryPlaceholderLabel"
Feel free to choose a different name -- and if there are already labels defined like this, group this new one together with them!
@@ -1,6 +1,7 @@ | |||
<template> | |||
<div v-bind:class="{ inline: inline === true, 'form-control': true }"> | |||
<div v-bind:class="{ inline: inline === true, 'form-control text-field': true }"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would separate the classes out; which makes it more verbose, yes, but also faster to parse that these are two classes
@@ -80,22 +80,26 @@ function fieldID (key: string): string { | |||
body { | |||
.radio-group-container { | |||
break-inside: avoid; | |||
margin: 10px 0; | |||
// margin: 10px 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to remove that line entirely if it isn't required!
grid-template-rows: 100%; | ||
grid-template-areas: "input label"; | ||
align-items: center; | ||
|
||
&:not(.checkbox-outer-div-inline) { | ||
margin: 10px 0; | ||
// padding: 5px 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to remove if no longer necessary
|
||
body.darwin { | ||
.form-control { | ||
>button { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>button { | |
> button { |
<div class="form-header"> | ||
<div | ||
class="form-header" | ||
v-bind:class="{ 'space-between': fieldset.title === 'Application language' }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really only necessary for a single field? This seems brittle, especially should we ever decide to change its name … feel free to explain what you want to achieve.
Made a number of small CSS tweaks to match the Preferences window implementation to the design spec.