Skip to content

Improve theme support for translate editing #3161

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

Merged
merged 12 commits into from
May 1, 2025

Conversation

nigel-wells
Copy link
Collaborator

@nigel-wells nigel-wells commented Apr 15, 2025

This PR improves theme support for a number of components used in the translation editor part of SF.

A number of colors in Quill have also been updated but not everything. Some colors will not be related to the main theme palette but do need attention to better support dark themes. This work could be done at a similar time as working on the notice component which also needs better support for dark mode.

One area in dark mode I didn't complete is the background color of Quill when not in read only mode. Currently it is still the same as light mode. This could be changed to a lighter neutral tone but that will also require changing other elements to better suit the new background - I made an initial start on supporting different colors in Quill based on read only mode.


This change is Reviewable

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.00%. Comparing base (da744a8) to head (0408239).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3161   +/-   ##
=======================================
  Coverage   83.00%   83.00%           
=======================================
  Files         569      569           
  Lines       33177    33177           
  Branches     5347     5347           
=======================================
  Hits        27540    27540           
  Misses       4833     4833           
  Partials      804      804           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephmyers josephmyers force-pushed the feature/theming-translate branch 2 times, most recently from 41aafe2 to dfaa059 Compare April 22, 2025 06:50
@josephmyers josephmyers force-pushed the feature/theming-translate branch 2 times, most recently from 793dc68 to 6b0a658 Compare April 30, 2025 06:03
@josephmyers josephmyers marked this pull request as ready for review April 30, 2025 06:04
@josephmyers
Copy link
Collaborator

This is one of those tasks that we could spend weeks on. Given the circumstances of the past few weeks, I'd like a second opinion on what I have so far.

@josephmyers josephmyers removed their assignment Apr 30, 2025
@RaymondLuong3 RaymondLuong3 self-assigned this Apr 30, 2025
nigel-wells and others added 11 commits April 30, 2025 10:34
This is to better align the different types of tabs in dark mode and to maintain good contrast with the different types of verses.

Removed the somewhat glaring purple from the toolbar in favor of a more muted theme, in line with light mode. Minor downside that the toolbar arguable blends in with the text content, but this too aligns with light mode.
It's easier to view now on dark mode
I changed the notice mode on the draft source page. The fill-dark error mode really clashes with all the purple on the screen. I also made the error color in app-notice slightly more red (other than the text). to help the modes that aren't "fill" to look redder and more like errors. These two changes are related.
I applied a custom theme and palette for the checkboxes for fill-dark and fill-light. The palette is generated, based off the colorDark of the notice type. It works well for these modes, which have a lighter background. The other modes seem to handle both light and dark themes well. I also tested most of the available types. There's no "is dark" detection logic needed, since the background color is the same between light and dark.

I also made the palettes public, so that they can be referenced in other files.
I opted for a minimal change for dark mode. Any dark background for the verses will require more of an overhaul to the editor's colors. Though I'm not opposed to this, it's more work than this acceptable (to me) path.
@RaymondLuong3 RaymondLuong3 force-pushed the feature/theming-translate branch from 6b0a658 to e9b9f8c Compare April 30, 2025 16:34
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

I looked through the text editor and the other changes here and all of the light mode colours looks good. I had a small suggestion for the note verse text colour. I didn't look too closely at the dark mode changes. We can incrementally refine those too.

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nigel-wells)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/note-dialog/note-dialog.component.scss line 34 at r1 (raw file):

  min-height: 30px;
  .text {
    color: var(--sf-note-dialog-reference-text-color);

I would like to see more contrast between the background and the text colour here.
image.png

Code quote:

    color: var(--sf-note-dialog-reference-text-color);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/biblical-terms/_biblical-terms-theme.scss line 6 at r1 (raw file):

  $is-dark: mat.get-theme-type($theme) == dark;

  --sf-biblical-terms-tabler-header-background: #{mat.get-theme-color($theme, primary, if($is-dark, 10, 95))};

I don't mind the slight colour on the table header, but I also would not be opposed to leaving it white.

Code quote:

  --sf-biblical-terms-tabler-header-background: #{mat.get-theme-color($theme, primary, if($is-dark, 10, 95))};

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

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

Thanks. I agree, dark mode is important, but not quite as important as light mode. We can tweak it as we see problems

Reviewable status: 19 of 20 files reviewed, 2 unresolved discussions (waiting on @nigel-wells and @RaymondLuong3)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/note-dialog/note-dialog.component.scss line 34 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I would like to see more contrast between the background and the text colour here.
image.png

Done.

@josephmyers josephmyers requested a review from RaymondLuong3 May 1, 2025 03:26
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nigel-wells)

@RaymondLuong3 RaymondLuong3 merged commit d39d943 into master May 1, 2025
17 of 18 checks passed
@RaymondLuong3 RaymondLuong3 deleted the feature/theming-translate branch May 1, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants