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

feat(lib): Inherits the dark/light theme's mode management using css variables and an ODS Charts DEFAULT mode #422

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Dec 5, 2024

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

NA

Description

TODO

Motivation & Context

Be able to use Boosted contextual dark mode to use ODS Charts dark mode. And handle the mode so it looks like Boosted v5.

Types of change

  • New feature (non-breaking change which adds functionality)

Test checklist

Please check that the following tests projects are still working:

  • docs/examples
  • test/angular-ngx-echarts
  • test/angular-echarts
  • test/html
  • test/react
  • test/vue
  • test/examples/bar-line-chart
  • test/examples/single-line-chart
  • test/examples/timeseries-chart

@louismaximepiton louismaximepiton force-pushed the main-lmp-handle-dark-mode-library branch 3 times, most recently from 980c81e to 59df041 Compare December 6, 2024 10:12
@jacques-lebourgeois
Copy link
Member

Perhaps it would be possible to do as in the example https://www.w3schools.com/howto/howto_html_include.asp to mutualize header and footer code? This is optional, perhaps to be dealt with at a later date.

Copy link

netlify bot commented Dec 13, 2024

Deploy Preview for ods-charts ready!

Name Link
🔨 Latest commit e00307f
🔍 Latest deploy log https://app.netlify.com/sites/ods-charts/deploys/678a6693f5dace0008c53615
😎 Deploy Preview https://deploy-preview-422--ods-charts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jacques-lebourgeois jacques-lebourgeois force-pushed the main-lmp-handle-dark-mode-library branch from 258b347 to acfb9ef Compare December 13, 2024 13:31
@jacques-lebourgeois jacques-lebourgeois changed the title Lib: Handle dark mode for Boosted v5 feat(lib): Inherits the dark/light theme's mode management using css variables and an ODS Charts DEFAULT mode Dec 13, 2024
@jacques-lebourgeois jacques-lebourgeois force-pushed the main-lmp-handle-dark-mode-library branch 3 times, most recently from a2aad1f to f605f4a Compare December 13, 2024 13:59
@jacques-lebourgeois jacques-lebourgeois changed the base branch from main to manage_canvas December 13, 2024 14:30
@jacques-lebourgeois jacques-lebourgeois marked this pull request as ready for review December 13, 2024 14:30
@jacques-lebourgeois jacques-lebourgeois force-pushed the main-lmp-handle-dark-mode-library branch from f605f4a to 9516175 Compare December 13, 2024 15:14
Base automatically changed from manage_canvas to main December 30, 2024 14:14
@louismaximepiton louismaximepiton force-pushed the main-lmp-handle-dark-mode-library branch from 9516175 to f9a8f32 Compare December 30, 2024 14:19
Copy link
Member Author

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Shouldn't we remove the fact that we can filll in a mode ?

color: ['#B5E8F7', '#80CEEF', '#4BB4E6', '#3E9DD6', '#237ECA', '#085EBD'],
color: [
'var(--ods-blue-100, #B5E8F7)',
'var(--ods-blue-200, #80CEEF)',
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
'var(--ods-blue-200, #80CEEF)',
'var(--ods-blue-200)',

Choose a reason for hiding this comment

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

Now, we should be able to remove fallback value.

Maybe it could help the maintenance.

but we also can do it in another PR after the last one of the series ?

let retunedValue = css;
if (this.options.cssSelector && 'string' === typeof retunedValue && !!this.computedStyle) {
try {
const regex = /var\(([^,]*),(.*)\)/g;
Copy link
Member Author

Choose a reason for hiding this comment

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

shouldn't we have /$--[^:]*: ?([^;]*/g instead ? So if people change the CSS var, we keep their value ? If yes, same below.

Choose a reason for hiding this comment

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

After reflexion, we are dealing only with constants from the library like

export const DEFAULT_COLORS_PURPLE = {
  color: [
    'var(--ods-purple-1, #492191)',
    'var(--ods-purple-2, #6E4AA7)',
    'var(--ods-purple-3, #9373BD)',
    'var(--ods-purple-4, #A885D8)',
    'var(--ods-purple-5, #C1A4E4)',
    'var(--ods-purple-6, #D9C2F0)',
  ],
};

so we know that any var definition has a fallback value as long as we decide to do it like that.

The fallback value should never be used but it is just a security.

We can decide that to simplify maintenance we remove the fallback value and then we will need to add the other case without fallback.

I push a commit to deal both cases. I test with :

  • no default value 'var(--ods-purple-1)',
  • some spaces 'var( --ods-purple-2 ,#6E4AA7 )',
  • regular way 'var(--ods-purple-3, #9373BD)',

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.

2 participants