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

deprecate: deprecate widthScale/heightScale in favor of width/height #10202

Closed
wants to merge 50 commits into from
Closed

deprecate: deprecate widthScale/heightScale in favor of width/height #10202

wants to merge 50 commits into from

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Sep 1, 2024 •
edited
Loading

Related Issue: #6172

Summary

Refactor to consolidate width/height and widthScale/heightScale into a single property width with s / m / l / auto / full as options, and height with s / m / l. Deprecate widthScale and heightScale properties and the half value.

@github-actions github-actions bot added the chore Issues with changes that don't modify src or test files. label Sep 1, 2024
@Elijbet Elijbet changed the title Refactor: consolidating width and widthScale into a single property Refactor: consolidate width and widthScale into a single property Sep 1, 2024
@Elijbet Elijbet changed the title Refactor: consolidate width and widthScale into a single property refactor: consolidate width and widthScale into a single property Sep 1, 2024
@Elijbet Elijbet marked this pull request as ready for review September 1, 2024 21:22
@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Sep 4, 2024
@Elijbet Elijbet changed the title refactor: consolidate width and widthScale into a single property refactor: consolidate width/height and widthScale/heightScale into a single property Sep 4, 2024
@Elijbet Elijbet changed the title refactor: consolidate width/height and widthScale/heightScale into a single property feat: consolidate width/height and widthScale/heightScale into a single property Sep 4, 2024
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Sep 4, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Sep 18, 2024
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Sep 20, 2024
jcfranco
jcfranco approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Once feedback is addressed, this should be good to merge!

📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏
📏📐📏📏📏📐📏📏📐📐📏📏📐📐📐📏📏📐📐📐📏📐📐📐📐📏📐📏
📏📐📐📏📏📐📏📐📏📏📐📏📏📐📏📏📐📏📏📏📏📐📏📏📏📏📐📏
📏📐📏📐📏📐📏📐📏📏📐📏📏📐📏📏📐📏📏📏📏📐📐📐📏📏📐📏
📏📐📏📏📐📐📏📐📏📏📐📏📏📐📏📏📐📏📏📏📏📐📏📏📏📏📏📏
📏📐📏📏📏📐📏📏📐📐📏📏📐📐📐📏📏📐📐📐📏📐📐📐📐📏📐📏
📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏📏

packages/calcite-components/src/components/dropdown/resources.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/dynamicClasses.spec.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/dynamicClasses.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/dynamicClasses.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/dynamicClasses.spec.ts Outdated Show resolved Hide resolved
@Elijbet Elijbet removed the Stale Issues or pull requests that have not had recent activity. label Oct 31, 2024
@Elijbet Elijbet changed the title feat: deprecate widthScale/heightScale in favor of width/height refactor: deprecate widthScale/heightScale in favor of width/height Nov 1, 2024
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 1, 2024
@Elijbet Elijbet changed the title refactor: deprecate widthScale/heightScale in favor of width/height deprecate: deprecate widthScale/heightScale in favor of width/height Nov 1, 2024
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 8, 2024
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 8, 2024
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 13, 2024
eriklharper
eriklharper reviewed Nov 13, 2024
View reviewed changes
packages/calcite-components/src/components/interfaces.ts
Comment on lines 30 to +32
export type Scale = "s" | "m" | "l";
export type Status = "invalid" | "valid" | "idle";
export type Width = "auto" | "half" | "full";
export type Width = "s" | "m" | "l" | "auto" | "half" | "full";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you do export type Width = Scale & "auto" | "half" | "full";? Same for Height above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess Height would just be export type Height = Scale;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the second thought, I feel like it might make things a bit confusing to read. Height, Width, and Scale are different props even though they have an overlap in type (with type Scale). s/m/l for scale does something else for width/height. Should they just have their own type names? It hurts my eyes to read type Height = Scale :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested that purely for DRY reasons. Its really just a union type to keep from having to define "s" | "m" | "l" in 3 different places.

Copy link
Contributor

@eriklharper eriklharper Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But yeah for type Height = Scale, agree it does look weird, but it has more semantics at least. I'll let others chime in on it, curious to hear other opinions. @jcfranco

@Elijbet
Copy link
Contributor Author

Elijbet commented Nov 19, 2024

This PR has had test timeout issues on local machine and coupled with lumina migration it was cleaner to start a new branch deprecate: deprecate widthScale/heightScale in favor of width/height #10786

@Elijbet Elijbet closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull left review comments

@eriklharper eriklharper eriklharper left review comments

@jcfranco jcfranco jcfranco approved these changes

@macandcheese macandcheese Awaiting requested review from macandcheese

@benelan benelan Awaiting requested review from benelan benelan is a code owner

Assignees

@Elijbet Elijbet

Labels
chore Issues with changes that don't modify src or test files. enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing. refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Elijbet @jcfranco @eriklharper @driskull