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

[FIX] *: standardization of color representation #2163

Closed
wants to merge 5 commits into from

Conversation

rrahir
Copy link
Collaborator

@rrahir rrahir commented Mar 3, 2023

Description:

We handle colors as 'css-valid' representations, as such, several
repesentations are equivalent but cannot be compared by strict equality,
they need to be converted to the same representation. This commit
aims to unify to the uppercase hex representation as it follows the
representation yielded by the helper toHex which (at this point in
time) seems to be the 'rallying' point when manipulating colors.

Task: 3193882

Odoo task ID : TASK_ID

review checklist

  • feature is organized in plugin, or UI components
  • support of duplicate sheet (deep copy)
  • in model/core: ranges are Range object, and can be adapted (adaptRanges)
  • in model/UI: ranges are strings (to show the user)
  • undo-able commands (uses this.history.update)
  • multiuser-able commands (has inverse commands and transformations where needed)
  • new/updated/removed commands are documented
  • exportable in excel
  • translations (_lt("qmsdf %s", abc))
  • unit tested
  • clean commented code
  • track breaking changes
  • doc is rebuild (npm run doc)
  • status is correct in Odoo

@robodoo
Copy link
Collaborator

robodoo commented Mar 3, 2023

Copy link
Contributor

@hokolomopo hokolomopo left a comment

Choose a reason for hiding this comment

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

It's always nice to standardize things 👍

src/components/color_picker/color_picker.ts Show resolved Hide resolved
@@ -13,7 +13,7 @@ import { Range } from "./range";
* Intellisense from resolving it.
* See https://github.com/microsoft/TypeScript/issues/31940#issuecomment-841712377
*/
export type Alias = {};
export type Alias = {} & {};
Copy link
Contributor

@hokolomopo hokolomopo Mar 7, 2023

Choose a reason for hiding this comment

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

The typing on mouse hover seemse to be workign now in VS code on my pc 😊

While we're there we could take the occasion to correclty type the colors that we missed previously. For example the colors in the cell Style, or all the color constants.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a commit for this. I might have overlooked some occurences though

tests/helpers/color.test.ts Show resolved Hide resolved
@rrahir rrahir force-pushed the 16.0-fix-color-picker-choice-rar branch 4 times, most recently from 260f29e to a3b0cf8 Compare March 9, 2023 11:41
Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

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

only ° in a docstring

@@ -34,7 +37,10 @@ export function getNextColor() {

/**
* Converts any CSS color value to a standardized hex6 value.
* Accepts: hex3, hex6, hex8 and rgb (rgba is not supported)
* Accepts: hex3, hex6, hex8, rgb° and rgba°.
Copy link
Collaborator

Choose a reason for hiding this comment

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

° ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah oui...
needs to be rebased

@@ -104,7 +104,7 @@ export class CustomColorsPlugin extends UIPlugin {
// again
[...new Set([...usedColors, ...this.customColors])]
.filter(isColorValid)
.map((c) => toHex(c).toLowerCase())
.map((c) => toHex(c))
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be simplified to .map(toHex)

@@ -14,7 +14,7 @@
<ColorPicker
t-if="state.openedColorPicker === 'backgroundColor'"
onColorPicked="(color) => this.setColor(color, 'backgroundColor')"
currentColor="props.definition.brackground"
currentColor="props.definition.background"
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol

@rrahir rrahir force-pushed the 16.0-fix-color-picker-choice-rar branch 2 times, most recently from a8b19af to 4b1ad7a Compare March 31, 2023 11:20
rrahir added 5 commits March 31, 2023 13:21
The Alias introduced to prevent Intellisense to resolve simple aliases
was broken. As a reminder, it allows us for example to type specific
string as `UID` and prevent the variables to be noted as `string`later
on in the code.
We handle colors as 'css-valid' representations, as such, several
repesentations are equivalent but cannot be compared by strict equality,
they need to be converted to the same representation. This commit
aims to unify to the uppercase hex representation as it follows the
representation yielded by the helper `toHex` which (at this point in
time) seems to be the 'rallying' point when manipulating colors.

Task: 3193882
The color picker would not close when the user would click outside of it
for the design panels of gauge and scorecard charts. The duplication of
design panles is probably to blame here.
@rrahir rrahir force-pushed the 16.0-fix-color-picker-choice-rar branch from 4b1ad7a to ccdaaaa Compare March 31, 2023 12:12
@rrahir
Copy link
Collaborator Author

rrahir commented Mar 31, 2023

@LucasLefevre I rebased the branch and changes the docstring with the ° char to something more clear (I hope^^)

Copy link
Collaborator

@LucasLefevre LucasLefevre left a comment

Choose a reason for hiding this comment

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

robodoo r+

@LucasLefevre
Copy link
Collaborator

robodoo rebase-ff

@robodoo
Copy link
Collaborator

robodoo commented Mar 31, 2023

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request Mar 31, 2023
The Alias introduced to prevent Intellisense to resolve simple aliases
was broken. As a reminder, it allows us for example to type specific
string as `UID` and prevent the variables to be noted as `string`later
on in the code.

Part-of: #2163
robodoo pushed a commit that referenced this pull request Mar 31, 2023
We handle colors as 'css-valid' representations, as such, several
repesentations are equivalent but cannot be compared by strict equality,
they need to be converted to the same representation. This commit
aims to unify to the uppercase hex representation as it follows the
representation yielded by the helper `toHex` which (at this point in
time) seems to be the 'rallying' point when manipulating colors.

Task: 3193882
Part-of: #2163
robodoo pushed a commit that referenced this pull request Mar 31, 2023
robodoo pushed a commit that referenced this pull request Mar 31, 2023
The color picker would not close when the user would click outside of it
for the design panels of gauge and scorecard charts. The duplication of
design panles is probably to blame here.

Part-of: #2163
robodoo pushed a commit that referenced this pull request Mar 31, 2023
closes #2163

Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
@robodoo robodoo temporarily deployed to merge March 31, 2023 12:46 Inactive
@robodoo robodoo closed this Mar 31, 2023
@fw-bot
Copy link
Collaborator

fw-bot commented Apr 4, 2023

@rrahir @LucasLefevre this pull request has forward-port PRs awaiting action (not merged or closed):
#2300

@fw-bot fw-bot deleted the 16.0-fix-color-picker-choice-rar branch April 14, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants