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

High contrast mode for subtitle editor #841

Merged
merged 10 commits into from
Dec 19, 2022

Conversation

narickmann
Copy link
Contributor

  • subtitle view now has a working high contrast mode
  • added some new color-attributes (subtitle_segment_bg, subtitle_segment_border and subtitle_segment_text)
  • colors for subtitle elements adjusted (bg-color, text-color, border-color etc)

- subtitle view now has a working  high contrast mode
- added some new color-attributes (subtitle_segment_bg, subtitle_segment_border and subtitle_segment_text)
- colors for subtitle elements adjusted (bg-color, text-color, border-color etc)
@narickmann narickmann added the type:accessibility This would help impaired users label Oct 21, 2022
@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/841/2022-10-21_07-55-18/ .
It might take a few minutes for it to become available.

Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

The high contrast mode lowers the contrast for the subtitle editor's video selector. The label is not legible at all anymore and the border is also no longer visible, making it hard to understand that this is a dropdown menu:

Screenshot from 2022-10-28 17-02-13

In high contrast mode, the language selector is literally black on black and you have to mark the text to read it:

Screenshot from 2022-10-28 16-59-32

The borders of the segment start and end times run into each other in high contrast mode. There should be at least a bit of margin between these fields:

Screenshot from 2022-10-28 17-00-45

- Added margin as suggested. Margin only applies in high contrast mode
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

This pull request is deployed at test.editor.opencast.org/841/2022-11-08_14-04-14/ .
It might take a few minutes for it to become available.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

The issues above all seem to have been addressed. Got one more related to the code.

const muiTheme = createTheme({
palette: {
mode: isDarkPreferred === 'dark' ? 'dark' : 'light',
const muiStyle = makeStyles({
Copy link
Member

Choose a reason for hiding this comment

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

This is already defined in SubtitleSelect.tsx. Can we define it in cssStyles.tsx instead to avoid duplication?

@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Nov 23, 2022
This patch removes duplicated style for the two dropdown menus in the subtitle area.

Unfortunately, I couldn't easily transfer the style to 'cssStyles.tsx' because the theme was then undefined and I couldn't work with the theme variables.
That's why I split it up like the calendar and used the css selectors.

I don't know if it would be worth the effort to create an extra component for these two menus (like for the tooltips) and then use that. But we could get rid of some css selectors.
@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/841/2022-11-24_15-36-21/ .
It might take a few minutes for it to become available.

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Nov 25, 2022
@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/841/2022-11-25_07-51-22/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member

Arnei commented Nov 30, 2022

The hint for the dropdowns does not show if the dropdown was in focus once and then isn't anymore. Only in dark-high-contrast mode. Probably because the color matches the background-color.
Screencast_30.11.2022_16:35:26.webm
(Same for the create dropdown in the select view)

Edit: Also applies to conventional dark mode.

The background-color of the dropdown menus ("Pick a language" and "Video Flavor" in subtitle view) was accidentally transparent on both high-contrast-modes.

The label is not visible after the dropdown was in focus once. This is only visible when you use a deploy link (like test.editor.opencast.org/841/2022-11-25_07-51-22/) I can't reproduce this on localhost.
Removed some css selectors from index.css that belong to the label but don't seem to have any effect.
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

This pull request is deployed at test.editor.opencast.org/841/2022-12-01_13-50-28/ .
It might take a few minutes for it to become available.

@narickmann
Copy link
Contributor Author

Last commit didn't fixed the label-issue. Can't reproduce this on localhost (on localhost it looks "normal" for all color modes).
This is only visible when you use a deploy link.

I also noticed that the clock in the calendar (in the metadata view) looks normal via the deploy link (all color modes) but the colors are not correct via localhost (all color modes).

localhost:
grafik

Deploy-Link - https://test.editor.opencast.org/841/2022-12-01_13-50-28/ :
grafik

@Arnei
Copy link
Member

Arnei commented Dec 2, 2022

Seems like running the app with start or building with build results in slightly different css-classes and, more importantly for us, different css-class-names. Since we hard-code css-class-names for the MUI elements, using a class name that is only present in one of the builds will result in the rule missing from the other.

It would still be extremely great if we didn't have to rely on hard-coding values for automatically generated class names.

Removed all hardcoded CSS class names from index.css and added the style for the two dropdown menus in subtitle view in cssStyles.tsx.
Did the same for the calendar and clock in metadata view.
Now both styles are separated. They should no longer affect each other due to the same CSS class names.
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

This pull request is deployed at test.editor.opencast.org/841/2022-12-08_09-07-25/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member

Arnei commented Dec 9, 2022

Somewhat nitpicky, but in regarding the clock view in the calendar: In light high contrast mode, for the arrows to switch between hours and minutes, the contrast is not very high (they are black when you can't click them, and dark blue when you can).

Looks good otherwise. Very happy you managed to get rid of the hard-coded class names.

Made the color a little lighter. It should now be more obvious if/that the arrow is disabled.
@github-actions
Copy link

github-actions bot commented Dec 9, 2022

This pull request is deployed at test.editor.opencast.org/841/2022-12-09_13-47-19/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member

Arnei commented Dec 9, 2022

Eh, good enough for me. Gonna wait a bit with merging to see if anyone else wants to take a look at this, but I'd say this overdue for merging.

@Arnei Arnei merged commit 5bb6672 into opencast:main Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:accessibility This would help impaired users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants