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(label): deprecate status prop #3984

Merged
merged 35 commits into from
Jan 27, 2022
Merged

feat(label): deprecate status prop #3984

merged 35 commits into from
Jan 27, 2022

Conversation

y0n4
Copy link
Contributor

@y0n4 y0n4 commented Jan 22, 2022

Related Issue: #3755

Summary

Originally user can set status indirectly by declaring status prop in calcite-input

<calcite-label status="invalid">
  <calcite-input></calcite-input>
  <calcite-input-message>Invalid</calcite-input-message>
</calcite-label>

However it is not ideal so to set status in calcite-input and calcite-input-message, the prop must be declared directly within those components. The status in calcite-label will be deprecated.

<calcite-label>
  <calcite-input status="invalid"></calcite-input>
  <calcite-input-message status="invalid">Invalid</calcite-input-message>
</calcite-label>

@y0n4 y0n4 requested a review from a team as a code owner January 22, 2022 00:28
@github-actions github-actions bot added this to the Sprint 01/17 - 01/28 milestone Jan 22, 2022
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 22, 2022
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

@y0n4 what about removing or deprecating the status property on the label?

Would this be a breaking change if we remove the getElementProp support on the inputs?

@y0n4 y0n4 changed the title fix(input, input-message): remove getElementProp for status and change verbiage on readme BREAKING CHANGE(input, input-message): remove getElementProp for status and change verbiage on readme Jan 24, 2022
@benelan benelan changed the title BREAKING CHANGE(input, input-message): remove getElementProp for status and change verbiage on readme fix(input, input-message)!: remove getElementProp for status and change verbiage on readme Jan 24, 2022
@y0n4 y0n4 changed the title fix(input, input-message)!: remove getElementProp for status and change verbiage on readme fix(input, input-message)!: deprecate and no longer support status prop for calcite-label Jan 24, 2022
@y0n4 y0n4 changed the title fix(input, input-message)!: deprecate and no longer support status prop for calcite-label fix(label, input, input-message)!: deprecate and no longer support status prop for calcite-label Jan 24, 2022
Yona Nagayama and others added 15 commits January 24, 2022 11:26
… slots. (#3962)

* refactor(panel, shell, tree-item): Add keys to conditionally rendered slots.

* add const for key.

* fix code

* review cleanup
Co-authored-by: jcfranco <jcfranco@users.noreply.github.com>
…3889)

* refactor: Update relevant components to use ConditionalSlotComponent #3686

* cleanup

* cleanup

* cleanup

* fix links that aren't direct slots

* remove assigned slot check, only get direct children

* fix test

* cleanup

* cleanup

* partial review fixes

* revert changes to combobox and card
…ed (#3969)

* refactor(slider): remove eventListeners on DOM disconnected

* feedback changes

* add private method for duplicate code

* rename method
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ame time most recently pressed key takes over (#3989)

Co-authored-by: Eliza Khachatryan <eli97736@esri.com>
@y0n4 y0n4 force-pushed the y0n4/3755-input-status branch from ee0f2bf to c18cdac Compare January 24, 2022 19:27
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.

Sorry for not catching this sooner, but since we are deprecating the label prop, we should still have it work. We will just have the examples and doc not promote the deprecated way. That means this will no longer be a breaking change, so no need for the ! in the PR title nor the BREAKING CHANGES footer when merging.

So, can you do the following?

  1. Restore the input and message (not label) e2e tests and update the test name with a (deprecated) or add a comment to the assertions (if the test title isn't specific to status) to indicate that it is deprecated.
  2. Restore the this.status = getElementProp(this.el, "status", this.status); lines.

I'll approve after these changes are made.

@driskull
Copy link
Member

Thanks for chiming in! I wasn't sure if we wanted to deprecate or if we were ok removing it and allowing a breaking change.

I guess the issue should have clarified a deprecation instead of removal.

eriklharper and others added 14 commits January 27, 2022 14:15
* docs(templates): provide content for the repro samples

* use cc latest version

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…l code is not found (#3968)

* fix(date-picker): update utils locale to get the lang code if regional code is not found

* alter util changes based from pr comments
* ci: allow primary and secondary contacts to merge on all prs

* cleanup

* use admin token
…ion files (#3980)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix(tree): enable multi select with pointer

* save selected items at main level

* fix single selection mode

* deselect siblings when mode is single

* fix deselecting tree item when mode is multi

* fix deselecting with shift keydown

* refactor e2e test

* remove unused imports in e2e

* feedback changes

* emit calciteTreeSelect event without shift key

* refactor code

* add e2e test and feedback changes

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* test(dropdown): correct logical errors in the test

* assert both events after each toggle action and  use page.waitForEvent before doing assertions

Co-authored-by: Eliza Khachatryan <eli97736@esri.com>
@y0n4 y0n4 changed the title fix(label, input, input-message)!: deprecate and no longer support status prop for calcite-label fix(label, input, input-message): deprecate and no longer support status prop for calcite-label Jan 27, 2022
@jcfranco jcfranco changed the title fix(label, input, input-message): deprecate and no longer support status prop for calcite-label feat(label): deprecate status prop Jan 27, 2022
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.

👍🏼

I renamed the PR title according to this thread.

@@ -37,8 +37,8 @@ describe("calcite-input-message", () => {
</calcite-label>
`);

const element = await page.find("calcite-input-message");
expect(element).toEqualAttribute("status", "invalid");
const deprecatedLabelStatusElement = await page.find("calcite-input-message");
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

/** specify the status of the label and any child input / input messages */
/**
* specify the status of the label and any child input / input messages
* @deprecated set directly on child element instead
Copy link
Member

Choose a reason for hiding this comment

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

+@benelan for editorial glance. 👀

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, thanks for changing it Yona!

Copy link
Member

@benelan benelan Jan 27, 2022

Choose a reason for hiding this comment

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

Although the prop description is kinda confusing to me (I'll do an audit at some point before v1) but that was already there lol. The new deprecation notice is 👨‍🍳 👌

@y0n4 y0n4 merged commit 066571f into master Jan 27, 2022
@y0n4 y0n4 deleted the y0n4/3755-input-status branch January 27, 2022 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants