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

[55775] Follow-up issues for the Dark mode #16002

Merged
merged 19 commits into from
Jul 8, 2024

Conversation

bsatarnejad
Copy link
Contributor

@bsatarnejad bsatarnejad requested a review from HDinger July 2, 2024 14:50
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

While testing the checked issues in the ticket, I found some things that are not working as expected:

  • On the overview page, the graph text is now readable. However, the grid itself is not visible:
Bildschirmfoto 2024-07-03 um 07 57 42 Bildschirmfoto 2024-07-03 um 07 57 54
  • The input on the repository page is still white
Bildschirmfoto 2024-07-03 um 07 58 36
  • The table on the cost reports page is also white
Bildschirmfoto 2024-07-03 um 07 59 08
  • Hovering over an attached file (e.g. in documents) still has the white hover effect
Bildschirmfoto 2024-07-03 um 08 01 17
  • When trying to enable a project attribute in a new project I still don't see which projects are already selected and thus disabled in the dropdown (in the screen shot below, "Seeded project" is disabled)
Bildschirmfoto 2024-07-03 um 08 03 57

Comment on lines 23 to 35
select
-webkit-appearance: none !important
-moz-appearance: none !important
background: var(--body-background) !important
background-image: var(--select-arrow-bg-color-url) !important
background-repeat: no-repeat !important
background-position-x: 100% !important
background-position-y: 5px !important
margin-right: 2rem !important
padding-right: 2rem !important

&>option
background-color: var(--body-background)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please double check whether all of this (and the new variable) is really needed. We already have a similar thing here. Ideally, you'd only have to change the $select-arrow-color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @HDinger,
thanks for the hints, image-triangle() function only receives colors like '#000', we can't pass a variable to it and $select-arrow-color is not a global variable, so I can't map it to another value in primer-mapping file.

@bsatarnejad bsatarnejad requested a review from HDinger July 3, 2024 12:01
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @bsatarnejad

I have only some minor remarks left 👍

@@ -157,6 +156,9 @@ select

&:not(.FormControl-select)
@if $select-arrow
background: transparent url(image-triangle($select-arrow-color)) right 10px center no-repeat
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the image-triangle function as well as it is not used anywhere else

@@ -112,7 +112,7 @@ div.autocomplete
border-color: var(--borderColor-default) !important

.ng-select-container
background-color: transparent !important
background-color: var(--body-background) !important
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this change, the select fields are now again darker than the other input fields

Bildschirmfoto 2024-07-04 um 08 43 21

@@ -98,10 +98,32 @@ export class WorkPackageEmbeddedGraphComponent {
}

protected setChartOptions() {
const bodyFontColor= getComputedStyle(document.body).getPropertyValue('--body-font-color');
const gridLineColor= getComputedStyle(document.body).getPropertyValue('--borderColor-default');
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that I brought this example of th doughnut chart up, but I just checked on edge and there, the grid and the scales are not visible at all for these kind of diagrams. There are only shown for some (like the bar chart)

Bildschirmfoto 2024-07-04 um 08 44 29

@bsatarnejad bsatarnejad force-pushed the 55775-follow-up-issues-for-the-Dark-mode branch from 065a8b9 to 281363b Compare July 4, 2024 07:15
@bsatarnejad bsatarnejad changed the base branch from dev to release/14.3 July 4, 2024 07:16
@bsatarnejad bsatarnejad requested a review from HDinger July 4, 2024 12:21
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @bsatarnejad

I have only one remark left 👍

@@ -187,4 +187,8 @@ export class WorkPackageEmbeddedGraphComponent {
this.chartHeight = '100%';
}
}

private isBarChart() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that Bar charts are not the only ones in which we want to show the axis. I think we want to show them for all types except "pie" and "doughnut" (like it was before)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @HDinger
I checked stage, we only show the grid line for bar, horizontal bar and line graphs.
There were no grid lines for pie, doughnut, polar area and radar graph types.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is right. I confused the lines of the radar and polar area with the axes. My bad 🙈

Nevertheless, there are some issues with this. In dark mode, the lines fir polar area and radar graph typesare invisible:
Bildschirmfoto 2024-07-05 um 07 51 09

and in the light mode, the pie and doughnut still display axes:
Bildschirmfoto 2024-07-05 um 07 50 25

@bsatarnejad bsatarnejad requested a review from HDinger July 5, 2024 09:22
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

There is still somthing wrong with the charts. In the light mode, I see a grid which helps me identifying the values. In dark mode, those lines are invisible
Bildschirmfoto 2024-07-08 um 08 13 42
Bildschirmfoto 2024-07-08 um 08 13 56

Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Forget my previous comment, I was on the wrong commit 🤦‍♀️ Everything is looking good 👍

@bsatarnejad bsatarnejad force-pushed the 55775-follow-up-issues-for-the-Dark-mode branch from b2f19ae to 8f45700 Compare July 8, 2024 06:39
@bsatarnejad bsatarnejad changed the base branch from release/14.3 to dev July 8, 2024 06:39
Copy link

github-actions bot commented Jul 8, 2024

1 Warning
⚠️ This PR has migration-related changes on a release branch. Ping @opf/operations

Generated by 🚫 Danger

@bsatarnejad bsatarnejad merged commit b0fcd01 into dev Jul 8, 2024
13 checks passed
@bsatarnejad bsatarnejad deleted the 55775-follow-up-issues-for-the-Dark-mode branch July 8, 2024 07:06
@oliverguenther oliverguenther restored the 55775-follow-up-issues-for-the-Dark-mode branch July 8, 2024 13:00
@oliverguenther oliverguenther deleted the 55775-follow-up-issues-for-the-Dark-mode branch July 8, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants