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

bugfix/15921-fillcolor-hcm #20703

Merged
merged 9 commits into from Mar 1, 2024
Merged

bugfix/15921-fillcolor-hcm #20703

merged 9 commits into from Mar 1, 2024

Conversation

goransle
Copy link
Member

@goransle goransle commented Feb 23, 2024

Fixed #15921 by applying fillColor to series from highContrastTheme.colors if set.

Also introduced highContrastMode option to allow disabling/forcing of high contrast mode.

WIP Fiddles:

TODO:

  • Internal review
  • Add sample
  • Add integration test

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Feb 23, 2024

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
modules/accessibility.js 37.3 kB
135.8 kB
37.4 kB
136.0 kB
76 B
224 B

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Feb 23, 2024

Visual test results - No difference found

@KacperMadej
Copy link

KacperMadej commented Feb 23, 2024

  • initial rendering of any Stock demo is wrongly styled and gets correct after a redraw - throws an error in the console
  • color axis also throws an error (e.g. basic map)
  • some paths with fill:'none' get one ('window') - e.g. flight-routes
    image

@goransle
Copy link
Member Author

goransle commented Feb 26, 2024

  • some paths with fill:'none' get one ('window') - e.g. flight-routes

Something interesting going on with series.setOptions here. Adding

        mappoint: {
            fillColor: 'none'
        },
        mapline: {
            fillColor: 'none'
        },
        map: {
            fillColor: 'none'
        }

to HighContrastTheme.ts produces this (map + mappoint added, but no mapline):
image

@goransle
Copy link
Member Author

⬆️ added as issue #20716

Copy link
Contributor

@marvin19 marvin19 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I think (if possible) to remove pattern fills and find colors with enough contrast for the demos. You can use white as the color in the top triangle e.g

@goransle goransle added the Changelog: Bugfix Use on PR to add description as a bugfix in the generated changelog. label Feb 29, 2024
@goransle goransle marked this pull request as ready for review February 29, 2024 13:09
Copy link
Member

@oysteinmoseng oysteinmoseng left a comment

Choose a reason for hiding this comment

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

Nice, just nitpick!

!chart.highContrastModeActive && // Only do this once
whcm.isHighContrastModeActive()
!chart.highContrastModeActive &&
!a11yOptions.highContrastMode === false && (
Copy link
Member

Choose a reason for hiding this comment

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

a11yOptions.highContrastMode !== false? (save 1 byte 🥳)

Copy link
Member Author

@goransle goransle Feb 29, 2024

Choose a reason for hiding this comment

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

🥳 / 😕
image

Copy link
Member Author

Choose a reason for hiding this comment

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

wait a minute...

Copy link
Member

Choose a reason for hiding this comment

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

Gzip works in mysterious ways.. (But code is easier to read anyway)

Copy link
Member

@oysteinmoseng oysteinmoseng left a comment

Choose a reason for hiding this comment

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

🥳

Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

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

Thanks!

@TorsteinHonsi TorsteinHonsi merged commit 3059b3e into master Mar 1, 2024
10 of 11 checks passed
@TorsteinHonsi TorsteinHonsi deleted the bugfix/15921-fillcolor-hcm branch March 1, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Use on PR to add description as a bugfix in the generated changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Area Chart not displaying correct colors in high contrast mode with theme set
6 participants