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

Move all theme colors to colors section #204

Merged
merged 2 commits into from
Jan 19, 2019

Conversation

nana-4
Copy link
Member

@nana-4 nana-4 commented Jan 17, 2019

All theme colors should be in Colors section for less user confusion.

All theme colors should be in Colors section for less user confusion.
{
'key': 'ARC_TRANSPARENCY',
'type': 'bool',
'fallback_value': True,
'display_name': _('Enable GTK3 Theme Transparency'),
'display_name': _('Enable Theme Transparency'),
Copy link
Member

@actionless actionless Jan 17, 2019

Choose a reason for hiding this comment

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

did they modified Arch theme what now it supports setting border colors and transparency outside gtk3?

if not — then renaming those could be misleading since people will be creating tickets why the setting didn't applied to their gtk2 or xfce theme

Copy link
Member Author

@nana-4 nana-4 Jan 17, 2019

Choose a reason for hiding this comment

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

Copy link
Member Author

@nana-4 nana-4 Jan 17, 2019

Choose a reason for hiding this comment

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

Regarding the border color: At least $button_border is used in gnome-shell theme.

UPD: Actually $entry_border is also used in gnome-shell and cinnamon.

Off-topic: I didn't carefully check, and I thought $borders_color also has the ARC_WIDGET_BORDER_COLOR var, but it was wrong... Is that intentional?

Copy link
Member

@actionless actionless Jan 17, 2019

Choose a reason for hiding this comment

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

it should be used for both entry and button borders, so you found a bug in theme template :-)

so this change can be accepted in this case

still i would like to discuss what could be a good way to report in theme options list what some options available/unavailable in some specific desktop environments or toolkits

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

information-symbol -- exactly what should fit the case

Copy link
Member

Choose a reason for hiding this comment

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

asterisk could benefit as being less visual noisy, though it could be unclear

Copy link
Member

Choose a reason for hiding this comment

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

or warning icon with tooltip text not supported by GTK+2 theme

Copy link
Member

@actionless actionless Jan 18, 2019

Choose a reason for hiding this comment

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

also, should such tooltip happen on hover or on click?

Copy link
Member Author

Choose a reason for hiding this comment

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

or warning icon with tooltip text not supported by GTK+2 theme

That would be the best 👍

also, should such tooltip happen on hover or on click?

+1 for on hover :)

@@ -99,6 +99,21 @@ class Plugin(OomoxThemePlugin):
'GTK3_GENERATE_DARK',
]

theme_model_gtk = [
Copy link
Member

@actionless actionless Jan 17, 2019

Choose a reason for hiding this comment

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

i think for caret will be better to just create its own sub-section named "Text Input Caret" (under section "Theme", like in Oomox-gtk-theme it's currently done for GTK3-only options) after theme options will be splitted by StackSwitcher into main sections (Theme/Icons/Terminal/Other)

as i can judge from the mockups of settings screens and incorporating into textstyle recommendations (to avoid repeating labels and prefer grouping of those items instead) i think splitting theme options into smaller sub-section could make sense

Copy link
Member Author

@nana-4 nana-4 Jan 17, 2019

Choose a reason for hiding this comment

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

Personally I feel pretty strange that some color options are not in the Colors section.

What about moving "Caret Aspect Ratio" into the "Advanced Options" section as my latest mockup instead of that?

Copy link
Member

@actionless actionless Jan 17, 2019

Choose a reason for hiding this comment

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

i think rather than having some hardcoded options i think it would make sense to group some specific options, for example gtk3-only or caret-specific

but specifically with caret my main motivation -- i really tired of questions what the "caret" is, and when it's under its own section there it's written "Text Input Caret" it's at least giving a hint what the caret is and allows also to avoid repetitive "prefix" "Text Input Caret ..." in options names

idk, mb try to come up with shorter but cleaner labels to solve the issue i've illustrated above, mb "Text Caret 1", "Text Caret 2" in colors section and "Text Caret Aspect Ratio" in other options and also put caret color options close to textbox color options

Copy link
Member

Choose a reason for hiding this comment

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

or even Text Cursor instead of Text Caret

Copy link
Member Author

@nana-4 nana-4 Jan 17, 2019

Choose a reason for hiding this comment

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

As another option: What do you think about removing those "caret" options from Oomox theme? To be honest, I think those "caret" options are pretty niche. Even me, I don't know how meaningful/effective it is to change the "Caret Aspect Ratio". I think majority people don't understand what they are, while only quite a few people may need those options. (This is why I moved "Caret Aspect Ratio" into the "Advanced Options" section in my mockup...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Textbox Caret, BiDi Textbox Caret, and Textbox Caret Aspect Ratio?

Hmm, I personally feel Text Caret sounds more natural and enough, though I'm not good at English...

Copy link
Member Author

Choose a reason for hiding this comment

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

why does Bi-Directional caret require separate styling from a primary one?

See https://bugzilla.gnome.org/show_bug.cgi?id=764204

Copy link
Member

Choose a reason for hiding this comment

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

everyday you learn something new :-)

i don't have strong preference towards text/textbox but it just seems more inline with 'textbox text' and 'textbox background'

Copy link
Member

Choose a reason for hiding this comment

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

or just leave caret related stuff to live in it's own section below other gtk options, as now :)

Copy link
Member Author

@nana-4 nana-4 Jan 18, 2019

Choose a reason for hiding this comment

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

Renamed Caret options.

or just leave caret related stuff to live in it's own section below other gtk options, as now :)

One of my motivations of this PR is to make comparisons with other colors easier. In the current GUI, long scrolling is required when you want to compare Caret colors with other colors. That's a bit less accessible, especially when you want to sync the caret colors with other colors.

@actionless actionless merged commit ebe26ca into themix-project:master Jan 19, 2019
@actionless
Copy link
Member

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants