[Fix] "Guidelines" Documentation Render Blank Page

[willie-hung]

Description

Resolve the issue where the Source Documentation would render blank pages when users clicked on either the "Colors" or "Sass" sections.

Issues Resolved

Closes #1109

Screenshot

Before

before-1.mov

After

after.mov

Check List

• New functionality includes testing.
• New functionality has been documented.
• All tests pass
  • yarn lint
  • yarn test-unit
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[willie-hung]

While investigating this issue, I found it challenging to identify the bug due to the implicit typing of variables.

For example, the main issue lies in the fact that palette[color] is already a string in the format "#017D73". Therefore, the original code rgbToHex(palette[color].rgba).toUpperCase() is unnecessary and prone to errors.

Do we have any plans to migrate some of these files to TypeScript for better type safety?

[vvavdiya]

@Willie-The-Lord
PR details need to update in CHANGELOG.md

[willie-hung]

@Willie-The-Lord PR details need to update in CHANGELOG.md

Thank you for the heads up!

[BigSamu]

Apart from the minor comment I made, the changes LGTM! I have already reviewed the code and tested it locally. Indeed, the problem was tricky, as @Willie-The-Lord mentioned.

[BigSamu]

@BSFishy,

Additionally I want to comment the following when doing console.log(palette) for the aux function getSassVars(theme) in the file src-docs/src/views/guidelines/_get_sass_vars.js

Why do we repeat the colour themes using variables that have the suffix Text. For example, take a look at the screenshot below. We are repeating the values for hex colour"#F66" in the keyseuiDangerColor and euiDangerColorText. In both cases, the value is a string. Shouldn't we avoid DRY?

[BSFishy]

While investigating this issue, I found it challenging to identify the bug due to the implicit typing of variables.

For example, the main issue lies in the fact that palette[color] is already a string in the format "#017D73". Therefore, the original code rgbToHex(palette[color].rgba).toUpperCase() is unnecessary and prone to errors.

Do we have any plans to migrate some of these files to TypeScript for better type safety?

Unfortunately, not really. These variables are extracted from Sass, using custom scripts: https://github.com/opensearch-project/oui/blob/main/scripts/babel/variables-from-scss/index.js and https://github.com/opensearch-project/oui/blob/main/scripts/lib/compile-scss-with-variables.js.

I think the format changed from

color: {
  r: string;
  g: string;
  b: string;
  a: string;
  rgba: string;
}

into

color: string

Which is where the errors come from. We might be able to add some sort of type safety by updating the Webpack plugin.

Why do we repeat the colour themes using variables that have the suffix Text. For example, take a look at the screenshot below. We are repeating the values for hex colour"#F66" in the keyseuiDangerColor and euiDangerColorText. In both cases, the value is a string. Shouldn't we avoid DRY?

To this point, the generates vars are just all of the Sass vars in the projects put into a JSON object. So for whatever reason we have these variables which are used in different ways, all just extracted and put into a single object.

[BigSamu]

To this point, the generates vars are just all of the Sass vars in the projects put into a JSON object. So for whatever reason we have these variables which are used in different ways, all just extracted and put into a single object.

Just asking here if we should simplify the JSON object to avoid DRY. It does not make sense to have the same value for 2 distinct keys. Of course, this will mean refactoring the code in other parts.

Regards,

Samuel

[joshuarrrr]

While investigating this issue, I found it challenging to identify the bug due to the implicit typing of variables.

For example, the main issue lies in the fact that palette[color] is already a string in the format "#017D73". Therefore, the original code rgbToHex(palette[color].rgba).toUpperCase() is unnecessary and prone to errors.

Do we have any plans to migrate some of these files to TypeScript for better type safety?

I don't think we have any overall issue for migrating OUI from JS to TS, but I think you could open one with this as an example of why it may be worth the effort to do. The good thing about typescript conversions is that they can be parallelized and worked on as good first issues.

[joshuarrrr]

To this point, the generates vars are just all of the Sass vars in the projects put into a JSON object. So for whatever reason we have these variables which are used in different ways, all just extracted and put into a single object.

Just asking here if we should simplify the JSON object to avoid DRY. It does not make sense to have the same value for 2 distinct keys. Of course, this will mean refactoring the code in other parts.

Regards,

Samuel

@BigSamu This has to do with the way we use SASS vars in the project. In many cases, it's useful to have multiple aliases that map to different usages even if (in some themes) they actually use the same underlying value. When you look at the JSON file (or the output of _get_sass_vars.js), what you're seeing is the calculated/resolved values for each SASS var in the currently selected theme. But different themes (or even light/dark mode within a particular theme) may assign these values differently.

In your example, in that theme the euiColorDanger is suitable for use as a text color given the themed background colors, so it's not transformed by the makeHighContrastColor() function: https://github.com/search?q=repo%3Aopensearch-project%2Foui+ouiColorDangerText%3A&type=code

But each theme is free to define the relationship between the euiColorDanger and euiColorDangerText variables (even hard-coding both), and OUI components are structured to use the appropriate variable whether the same or different.

[willie-hung]

To this point, the generates vars are just all of the Sass vars in the projects put into a JSON object. So for whatever reason we have these variables which are used in different ways, all just extracted and put into a single object.

Just asking here if we should simplify the JSON object to avoid DRY. It does not make sense to have the same value for 2 distinct keys. Of course, this will mean refactoring the code in other parts.
Regards,
Samuel

@BigSamu This has to do with the way we use SASS vars in the project. In many cases, it's useful to have multiple aliases that map to different usages even if (in some themes) they actually use the same underlying value. When you look at the JSON file (or the output of _get_sass_vars.js), what you're seeing is the calculated/resolved values for each SASS var in the currently selected theme. But different themes (or even light/dark mode within a particular theme) may assign these values differently.

In your example, in that theme the euiColorDanger is suitable for use as a text color given the themed background colors, so it's not transformed by the makeHighContrastColor() function: https://github.com/search?q=repo%3Aopensearch-project%2Foui+ouiColorDangerText%3A&type=code

But each theme is free to define the relationship between the euiColorDanger and euiColorDangerText variables (even hard-coding both), and OUI components are structured to use the appropriate variable whether the same or different.

Thanks @joshuarrrr for the detailed explanation! It makes sense that the flexibility is beneficial, especially when we consider the implementation across various themes.

[joshuarrrr]

Awesome fix @Willie-The-Lord - just a couple minor questions and suggestions.

[BigSamu]

To this point, the generates vars are just all of the Sass vars in the projects put into a JSON object. So for whatever reason we have these variables which are used in different ways, all just extracted and put into a single object.

Just asking here if we should simplify the JSON object to avoid DRY. It does not make sense to have the same value for 2 distinct keys. Of course, this will mean refactoring the code in other parts.
Regards,
Samuel

@BigSamu This has to do with the way we use SASS vars in the project. In many cases, it's useful to have multiple aliases that map to different usages even if (in some themes) they actually use the same underlying value. When you look at the JSON file (or the output of _get_sass_vars.js), what you're seeing is the calculated/resolved values for each SASS var in the currently selected theme. But different themes (or even light/dark mode within a particular theme) may assign these values differently.

In your example, in that theme the euiColorDanger is suitable for use as a text color given the themed background colors, so it's not transformed by the makeHighContrastColor() function: https://github.com/search?q=repo%3Aopensearch-project%2Foui+ouiColorDangerText%3A&type=code

But each theme is free to define the relationship between the euiColorDanger and euiColorDangerText variables (even hard-coding both), and OUI components are structured to use the appropriate variable whether the same or different.

Many thanks @joshuarrrr! Pretty clear the explanation!