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.

Sign up for GitHub

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

Jump to bottom

Enhancement: Rename 'color' component to 'color-picker'. #1437

Closed
driskull opened this issue Dec 31, 2020 · 12 comments
Closed

Enhancement: Rename 'color' component to 'color-picker'. #1437

driskull opened this issue Dec 31, 2020 · 12 comments
Assignees
@julio8a
Labels
4 - verified Issues that have been released and confirmed resolved. breaking change Issues and pull requests with code changes that are not backwards compatible. convention Issues relating to conventions and best practices. discussion Issues relating to a conversation where feedback is optional. enhancement Issues tied to a new feature or request.
Milestone
v1.0.0-beta.51

Comments

@driskull
Copy link
Member

driskull commented Dec 31, 2020 •
edited
Loading

Description

For consistency and specificity, I think we should rename the color component to color-picker.

We should also consider renaming its child components.

  • calcite-color-hex-input => calcite-input-color-hex. // input first since it's an input component.
  • calcite-color-swatch => calcite-color-picker-swatch. If this could ever be used outside of this component then I would propose calcite-color-swatch.

Question(s)

  • Should we also create an input that launches this color picker? @julio8a is that in the plans? Something like calcite-input-color-picker?

Acceptance Criteria

calcite-color is consistent with calcite-time-picker and calcite-date-picker.

Relevant Info

Which Component

  • calcite-color

Example Use Case

NA.
@driskull driskull added enhancement Issues tied to a new feature or request. discussion Issues relating to a conversation where feedback is optional. convention Issues relating to conventions and best practices. breaking change Issues and pull requests with code changes that are not backwards compatible. 1 - assigned Issues that are assigned to a sprint and a team member. labels Dec 31, 2020
@driskull driskull added this to the v1.0.0-beta.48 milestone Dec 31, 2020
@jcfranco
Copy link
Member

jcfranco commented Jan 5, 2021

Follow-up questions:

  • Are we no longer using the main color-picker component name in supporting ones? E.g., calcite-color-picker-<support-component-name>
  • Does this need to be a breaking change? I thought we would deprecate calcite-color when calcite-color-picker is introduced.
  • I'd propose using calcite-hex-input vs calcite-input-color-hex since the pickers aren't following this convention: calcite-<thang>-picker instead of calcite-picker-<thang>
  • Could you add more info regarding the acceptance criteria? I'm not clear on how it should be consistent with other pickers.

Should we also create an input that launches this color picker? @julio8a is that in the plans? Something like calcite-input-color-picker?

For MVB, I was working w/ @mitc7862 on an inlined color component and he hashed out designs for a more generic component. I think it makes sense to add to this repo to allow this UX for other cases and not only color. I'll create an issue for it this week.

@driskull
Copy link
Member Author

driskull commented Jan 5, 2021

Are we no longer using the main color-picker component name in supporting ones? E.g., calcite-color-picker-

I think we can do that if hex will only be used by color picker.

Does this need to be a breaking change? I thought we would deprecate calcite-color when calcite-color-picker is introduced.

If we do it when we move to the new repo then it isn't a breaking change.

Could you add more info regarding the acceptance criteria? I'm not clear on how it should be consistent with other pickers.

Sure, we will have calcite-date-picker and calcite-time-picker. We will also have calcite-input-date-picker which is the inline input that opens the date picker.

@driskull
Copy link
Member Author

driskull commented Jan 5, 2021

I'd propose using calcite-hex-input vs calcite-input-color-hex since the pickers aren't following this convention: calcite--picker instead of calcite-picker-

If it's an input component, it might make more sense for the input to be first. That will allow us to group the input components and have consistency there. However, if it would only be used internally then there might be an exception here.

@macandcheese
Copy link
Contributor

I think the "input that launches the color picker" (with the preview swatch, that Mitch had specced) is valid as a "calcite-input-*", but that's separate from the current "hex input" correct?

@julio8a
Copy link

julio8a commented Feb 5, 2021

@jcfranco, from the meeting we had today, we should merge this one and not wait for 1.0.

@julio8a
Copy link

julio8a commented Feb 19, 2021

Checking back in on this, can we merge @jcfranco?

@jcfranco
Copy link
Member

I'll work on having a PR by tomorrow at the latest.

@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Feb 24, 2021
@jcfranco jcfranco mentioned this issue Feb 24, 2021
Merged
@driskull
Copy link
Member Author

Do we need an issue for the input version that opens a color picker or is there already one?

@jcfranco
Copy link
Member

I don't think we have one. I'll create it.

@jcfranco
Copy link
Member

☝️ #1616

@jcfranco jcfranco mentioned this issue Feb 26, 2021
Merged
@jcfranco
Copy link
Member

Installed.

calcite-color will now be calcite-color-picker in the upcoming next/beta releases. cc @pemberdom @subgan82 @ethanbdev

Event renames landing soon (#1624).

@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Feb 26, 2021
jcfranco added a commit that referenced this issue Feb 26, 2021
@jcfranco
refactor(color)!: rename color to color-picker (#1624)
6500242 
#1437 

BREAKING CHANGE: The following events were renamed:

* `calciteColorChange` -> `calciteColorPickerChange`
* `calciteColorHexInputChange` -> `calciteColorPickerHexInputChange`
@jcfranco jcfranco assigned julio8a and unassigned jcfranco Feb 26, 2021
@jcfranco
Copy link
Member

jcfranco commented Mar 10, 2021 •
edited
Loading

Verified locally w/ beta.51.

@jcfranco jcfranco added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@julio8a julio8a

Labels
4 - verified Issues that have been released and confirmed resolved. breaking change Issues and pull requests with code changes that are not backwards compatible. convention Issues relating to conventions and best practices. discussion Issues relating to a conversation where feedback is optional. enhancement Issues tied to a new feature or request.
Projects
None yet
Milestone
v1.0.0-beta.51
Development

No branches or pull requests
4 participants
@jcfranco @driskull @macandcheese @julio8a