Skip to content
This repository was archived by the owner on Feb 19, 2024. It is now read-only.

Conversation

@Danfro
Copy link

@Danfro Danfro commented Feb 11, 2023

Replace some hardcoded colors with system colors in about page

// #335280 is the new blue by Canonical
// https://github.com/CanonicalLtd/desktop-design/blob/master/Colour/colour.png
property string themableBlue: Theme.name == "Ubuntu.Components.Themes.SuruDark" ? UbuntuColors.blue : "#335280"
property string linkColor: " style=\"color:"+about_column.themableBlue+";\"" //' style="color:'+ about_column.themableBlue +';"'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The linkColor property is handy and should be still used in every html string later in the code because it encapsulate the color into the html style attribute, but it needs to point to theme.palette.normal.activity instead, like you pointed out. Maybe you just overlooked this 😅 but thanks for the PR, this color was not part of the theme palette yet when has been introduced here

Copy link
Author

Choose a reason for hiding this comment

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

Haha, yes, you are right. I was too much in "cleaning mode". Of course keeping the property and set the color once is much better. I will fix that.

qml/About.qml Outdated
textFormat: Text.RichText
font.underline: false
text: i18n.tr("Released under the terms of the GNU GPL v3.<br>Source code available on") + " <a style=\"text-decoration: none;color:"+about_column.themableBlue+";\" href=\"https://github.com/ernesst/ActivityTracker\">GitHub.com</a>"
text: i18n.tr("Released under the terms of the GNU GPL v3.<br>Source code available on") + " <a "+about_column.linkColor+" style=\"text-decoration: none;\" href=\"https://github.com/ernesst/ActivityTracker\">GitHub.com</a>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry if I'm nitpicking this too much, but is it fine to put the style attribute twice here? Did you tested it and it works?

Copy link
Author

Choose a reason for hiding this comment

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

  1. I did not add this, that is from the original code
  2. just tried it, seems to work, results in the github link not having a underline
  3. Since all the other links do have an underline, lets remove it here too to have a consistent layout

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. The original code had the style attribute only once in that line and had the color property explicit and just the color appended via about_column.themableBlue variable, while you used about_column.linkColor which brings in the full html style attribute with color property set.
  2. 👍🏻
  3. Oh, ok not sure why I did this, let's make it consistent.

Anyway, this LGTM

@mymike00 mymike00 merged commit 4bd0954 into ernesst:master Feb 14, 2023
@Danfro Danfro deleted the colors branch February 14, 2023 08:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants