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

XWIKI-13912: Impossible to control contrast of the panel headers on Flamingo #2497

Merged
merged 12 commits into from
Jan 18, 2024

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Oct 24, 2023

Jira

https://jira.xwiki.org/browse/XWIKI-13912

PR Changes

  • Added the panel header color to the color theme UI
  • Updated variable name to fit in their new context

Sereza7 and others added 7 commits July 13, 2023 14:28
…lamingo

* Added the panel header text color to the ThemeSheet.xml
* Changed the name of the variable to fit other variables in the tab
…lamingo

* Fixed colortheme files and homogeneized property names
…lamingo

* Fixed codestyle
* Fixed consistency
@xwiki-panel-header-bg: $theme.panelHeaderBackgroundColor;
@xwiki-panel-header-text: $theme.panelHeaderTextColor;
@panel-header-bg: $theme.panelHeaderBackgroundColor;
@panel-header-text: $theme.panelHeaderTextColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to (additionally) keep the old variable names for backwards-compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5587492 👍

@xwiki-panel-header-bg: $theme.panelHeaderBackgroundColor;
@xwiki-panel-header-text: $theme.panelHeaderTextColor;
@panel-header-bg: $theme.panelHeaderBackgroundColor;
@panel-header-text: $theme.panelHeaderTextColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, wouldn't it be better to (additionally) keep the old variable names for backwards-compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5587492 👍

@michitux michitux requested a review from lucaa October 27, 2023 13:56
Comment on lines 104 to 105
@panel-header-bg: $theme.panelHeaderBackgroundColor;
@panel-header-text: $theme.panelHeaderTextColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong less variable name, this should be @xwiki-panel-header-bg/-text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I did not check my changes properly before commiting. Fixed in dc4c9d5

@michitux
Copy link
Contributor

@Sereza7 Could you please resolve the conflicts due to the merge of #2485?

@Sereza7
Copy link
Contributor Author

Sereza7 commented Oct 27, 2023

Yes, it was a bit lengthy ^^'

Copy link
Contributor

@michitux michitux left a comment

Choose a reason for hiding this comment

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

Looks good overall, but there is one theme file that looks wrong, see my comment.

@@ -2050,6 +2063,9 @@ a.list-group-item {
<property>
<panel-bg>#060606</panel-bg>
</property>
<property>
<panel-default-text>#ffffff</panel-default-text>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a bad merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 9c640c2 👍

@@ -2125,7 +2138,7 @@ div.main {
box-shadow: none;
}

@xwiki-panel-header-text: @navbar-default-color;
@panel-header-text: @navbar-default-color;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this either a) set the corresponding theme variable or b) also set the fallback value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For better backwards compatibility, and consistency with the choice we made in variablesInit.vm, I added back the former value as a fallback in afc21a6 👍

@Sereza7 Sereza7 marked this pull request as draft December 4, 2023 10:57
@Sereza7 Sereza7 marked this pull request as ready for review January 11, 2024 16:05
@surli surli merged commit d883277 into xwiki:master Jan 18, 2024
vmassol pushed a commit that referenced this pull request Jan 18, 2024
…lamingo (#2497)

* Added the panel header text color to the ThemeSheet.xml
* Changed the name of the variable to fit other variables in the tab
* Updated color themes
* Fixed property name
* Fixed colortheme files and homogeneized property names
* Fixed codestyle
* Fixed consistency
* Fixed backward compatibility
Sereza7 added a commit to Sereza7/xwiki-platform that referenced this pull request Jan 18, 2024
…lamingo (xwiki#2497)

* Added the panel header text color to the ThemeSheet.xml
* Changed the name of the variable to fit other variables in the tab
* Updated color themes
* Fixed property name
* Fixed colortheme files and homogeneized property names
* Fixed codestyle
* Fixed consistency
* Fixed backward compatibility
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.

3 participants