Dark Mode: Code Block in Test Report Details has bad colors
Summary
The <pre class="code-block rounded">
element in the modal window opened after clicking the View details
button in CI/CD Pipeline's test report is not readable in dark theme.
Steps to reproduce
- Swith to the
Dark Mode (alpha)
- Open any Test Report with a failed test containing some details about the failure
- Click
View details
Example Project
https://gitlab.com/himura/gitlab-test-reports/-/pipelines/322695440/test_report
What is the current bug behavior?
.code-block {
background: #fff;
color: #c4c4c4;
}
What is the expected correct behavior?
The code background is dark.
Relevant logs and/or screenshots
Results of GitLab environment info
Both GitLab.com and Self-hosted versions.
Possible fixes
Designs
- Show closed items
Relates to
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- Himura Kazuto changed title from Dark theme: Code block in test report details has bad colors to Dark Mode: Code Block in Test Report Details has bad colors
changed title from Dark theme: Code block in test report details has bad colors to Dark Mode: Code Block in Test Report Details has bad colors
- 🤖 GitLab Bot 🤖 added automation:ml grouppipeline security labels
added automation:ml grouppipeline security labels
- Maintainer
This issue was automatically tagged with the label grouptesting by TanukiStan, a machine learning classification model, with a probability of 0.64.
If this label is incorrect, please tag this issue with the correct group label as well as automation:ml wrong to help me learn from my mistakes.
- Maintainer
Setting label(s) devopsverify sectionops based on grouptesting.
- 🤖 GitLab Bot 🤖 added devopsverify sectionops labels
added devopsverify sectionops labels
- 🤖 GitLab Bot 🤖 mentioned in issue gitlab-org/quality/triage-reports#3559 (closed)
mentioned in issue gitlab-org/quality/triage-reports#3559 (closed)
- James Heimbuck added Category:Code Testing and Coverage UX debt UX frontend labels
added Category:Code Testing and Coverage UX debt UX frontend labels
- James Heimbuck changed milestone to %Backlog
changed milestone to %Backlog
- 🤖 GitLab Bot 🤖 added [deprecated] Accepting merge requests label
added [deprecated] Accepting merge requests label
Happening with any code block:
test code block
Collapse replies - Contributor
@CarlosAlbaladejo code blocks seem to respect whatever the user has selected as their preferred Syntax Highlighting
1 This works well for me now
🙌
- Rayana Verissimo added UI polish + 1 deleted label
added UI polish + 1 deleted label
- Rayana Verissimo removed UX debt label
removed UX debt label
- Developer
Thanks @himura for creating this issue!
@tauriedavis @jeldergl I think this is on your lane. Could you please help assess this issue? Thanks!
Collapse replies - Developer
Thanks @rayana. Foundations currently does not have capacity to take on dark mode at this time. That said, our direction is to create a systematized approach to dark mode UI that can scale across components. Currently, dark mode is in alpha so the expectation is that there may be bugs or issues such as this one. We really do hope to get to dark mode in the near future, but currently do not have anything planned.
I will add this issue to the epic tracking dark mode improvements.
- Contributor
@tauriedavis @rayana would an MVC approach to this be use either the default OR whatever syntax highlighting theme the user has selected?
cc @shampton @mfluharty as this applies to #336976 (comment 647849656) as well.
- Maintainer
would an MVC approach to this be use either the default
Making these blocks use a dark theme in dark mode and a light theme in normal mode might be as easy as removing these two lines to let the blocks use default dark/light styling instead of specific color definitions.
OR whatever syntax highlighting theme the user has selected?
We could make this use the user's code theme by adding the corresponding theme class and a
code
class to the<pre />
incode_block.vue
:diff --git a/app/assets/javascripts/vue_shared/components/code_block.vue b/app/assets/javascripts/vue_shared/components/code_block.vue index 1928bf6dac5..9856f35c7f6 100644 --- a/app/assets/javascripts/vue_shared/components/code_block.vue +++ b/app/assets/javascripts/vue_shared/components/code_block.vue @@ -24,8 +24,13 @@ export default { return isScrollable ? scrollableStyles : null; }, }, + userColorScheme: window.gon.user_color_scheme, }; </script> <template> - <pre class="code-block rounded" :style="styleObject"><code class="d-block">{{ code }}</code></pre> + <pre + class="code-block rounded code" + :class="$options.userColorScheme" + :style="styleObject" + ><code class="d-block">{{ code }}</code></pre> </template>
Either of these approaches would affect all
<code-block />
s, so we'd also have to figure out if that's what we want in these places:/cc @shampton
1 - Contributor
Thanks for the update @mfluharty we'll want to loop in the folks in groupsource code and groupstatic analysis since it looks like some of those components touch their features. cc @sarahwaldner @mvanremmerden @tmccaslin @jmandell.
1 - Developer
This is going to be groupthreat insights. We've seen a few other dark mode bugs in vuln management UI.
- Contributor
@jheimbuck_gl is this a ~bug for featureenhancement? If these are frontend -only changes, that might be something the groupthreat insights team can pick up in the near future.
- Contributor
@matt_wilson it's a ~bug and i'll apply the label. I think @mfluharty is saying if we make the change for the test details we'll impact all the other areas as well so you'd get this "free" but wanted to make sure you're in the loop / can review along with us.
- Contributor
@jheimbuck_gl Ah OK, I wasn't sure if this was something for us to change ourselves or just review. We can definitely review
👍 - Contributor
Thanks @matt_wilson
@mfluharty can you make sure we get someone from groupthreat insights as a reviewer on the MR for this issue once it's ready? I'm guessing that'd be @beckalippert
2 1
- Taurie Davis added to epic &2902
added to epic &2902
- Scott Hampton marked #336976 (closed) as a duplicate of this issue
marked #336976 (closed) as a duplicate of this issue
- Scott Hampton marked this issue as related to #336976 (closed)
marked this issue as related to #336976 (closed)
- Scott Hampton mentioned in issue #336976 (closed)
mentioned in issue #336976 (closed)
- James Heimbuck added usability label
added usability label
- James Heimbuck changed milestone to %14.4
changed milestone to %14.4
- Scott Hampton changed milestone to %14.3
changed milestone to %14.3
- Maintainer
@mfluharty I'm moving this into %14.3. Can you add a weight to this and when you feel comfortable, update the workflow label to workflowready for development?
Feel free to unassign yourself once the above is done.
/cc @jheimbuck_gl
1 1 Collapse replies - Maintainer
Proposal:
Let's stay consistent with the rest of the UI and use the user's syntax highlighting theme in this case.
We'd just need to add the corresponding theme class and a code class to code_block.vue
diff --git a/app/assets/javascripts/vue_shared/components/code_block.vue b/app/assets/javascripts/vue_shared/components/code_block.vue index 1928bf6dac5..9856f35c7f6 100644 --- a/app/assets/javascripts/vue_shared/components/code_block.vue +++ b/app/assets/javascripts/vue_shared/components/code_block.vue @@ -24,8 +24,13 @@ export default { return isScrollable ? scrollableStyles : null; }, }, + userColorScheme: window.gon.user_color_scheme, }; </script> <template> - <pre class="code-block rounded" :style="styleObject"><code class="d-block">{{ code }}</code></pre> + <pre + class="code-block rounded code" + :class="$options.userColorScheme" + :style="styleObject" + ><code class="d-block">{{ code }}</code></pre> </template>
That change alone would be a 1, but because it will also change any other instances of the
<code-block />
component and we'll have to check that it works correctly in all cases, I'll give this a weight of 2.1
- Scott Hampton assigned to @mfluharty
assigned to @mfluharty
- 🤖 GitLab Bot 🤖 removed [deprecated] Accepting merge requests label
removed [deprecated] Accepting merge requests label
- James Heimbuck added FY22Q3 label
added FY22Q3 label
- Rayana Verissimo removed typebug label
removed typebug label
- Developer
Removing the ~bug label since this is a UI polish .
FYI Here's how we apply labels to UX issues.
- James Heimbuck added workflowplanning breakdown label
added workflowplanning breakdown label
- Miranda Fluharty added workflowready for development label and removed workflowplanning breakdown label
added workflowready for development label and removed workflowplanning breakdown label
- Miranda Fluharty set weight to 2
set weight to 2
- Miranda Fluharty unassigned @mfluharty
unassigned @mfluharty
- 🤖 GitLab Bot 🤖 added [deprecated] Accepting merge requests label
added [deprecated] Accepting merge requests label
- James Heimbuck added Deliverable label
added Deliverable label
- Miranda Fluharty added workflowin dev label and removed workflowready for development label
added workflowin dev label and removed workflowready for development label
- Miranda Fluharty assigned to @mfluharty
assigned to @mfluharty
- Miranda Fluharty mentioned in merge request !68788 (merged)
mentioned in merge request !68788 (merged)
- 🤖 GitLab Bot 🤖 removed [deprecated] Accepting merge requests label
removed [deprecated] Accepting merge requests label
- James Heimbuck mentioned in issue gitlab-com/Product#2872 (closed)
mentioned in issue gitlab-com/Product#2872 (closed)
- Miranda Fluharty added workflowin review label and removed workflowin dev label
added workflowin review label and removed workflowin dev label
- Miranda Fluharty added workflowverification label and removed workflowin review label
added workflowverification label and removed workflowin review label
- Maintainer
Update 2021-09-07
!68788 (merged), which applies the user's selected syntax highlighting theme to these code blocks, has been merged and deployed
🎉 you can see it in action by clicking the "View details" button next to failed tests in our own pipeline test report, or next to any test in the example pipeline test report.Since this is a shared component, this also applies to code blocks in other locations, and all of them should be more readable in dark mode now. See the body of the merge request for details and screenshots of all the code blocks.
Closing this issue
1 Collapse replies - Maintainer
Fantastic job @mfluharty!
💯
- Miranda Fluharty closed
closed
- James Heimbuck mentioned in issue #24027 (closed)
mentioned in issue #24027 (closed)
- Jeff Tucker changed epic to &12573
changed epic to &12573