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

refactor: use calciteInternal prefix for internal events #4496

Merged

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented May 6, 2022

Related Issue: #2256

Summary

Renaming internal events to use calciteInternal prefix and stopping them from propagating when handled (where propagation is not needed).

List of Internal Events revised:

calciteAccordionItemKeyEvent
calciteAccordionItemSelect
calciteAccordionItemClose
calciteItemRegister
calciteAlertSync
calciteAlertRegister
calciteComboboxOpen
calciteComboboxClose
calciteDayHover
calciteDatePickerHover
calciteDatePickerMouseOut
calciteDropdownItemChange
calciteDropdownItemSelect
calciteDropdownItemKeyEvent
calciteDropdownCloseRequest
calciteInlineEditableEnableEditingChange
stop and check if all green first
calciteInputFocus
calciteInputBlur
calciteInputDatePickerOpen
calciteInputDatePickerClose
calciteInternalLabelClick
calciteOptionChange
calciteOptionGroupChange
calciteListItemPropsChange
calciteListItemValueChange
calciteInternalRadioButtonBlur
calciteInternalRadioButtonCheckedChange
calciteInternalRadioButtonFocus
calciteRadioGroupItemChange
calciteStepperItemChange
calciteStepperItemKeyEvent
calciteStepperItemSelect
calciteStepperItemRegister
calciteTabRegister
calciteInternalTabChange
calciteInternalTabsActivate
calciteTabsFocusNext
calciteTabsFocusPrevious
calciteTabTitleRegister
calciteTabTitleRegister
calciteTimePickerBlur
calciteTimePickerChange
calciteTimePickerFocus
calciteTreeItemSelect

@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label May 6, 2022
@Elijbet Elijbet changed the title refactor(all): use calciteInternal prefix for internal events refactor: use calciteInternal prefix for internal events May 6, 2022
@Elijbet Elijbet marked this pull request as ready for review May 11, 2022 22:52
@Elijbet Elijbet requested a review from a team as a code owner May 11, 2022 22:52
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.

Looks good! 💯

@@ -257,13 +257,13 @@ export class Combobox implements LabelableComponent, FormComponent, InteractiveC
* Fired when the combobox is opened
* @internal
*/
@Event() calciteComboboxOpen: EventEmitter;
@Event() calciteInternalComboboxOpen: EventEmitter;
Copy link
Member

Choose a reason for hiding this comment

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

These open/close ones might be useful for users at some point.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we be removing @internal from Open/Close events then and making them public? Or did you mean specifically for combobox?

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur with @driskull on this one too. Open/Close events should probably be publicly exposed. Especially since this is a top-level component that isn't emitting this event for another Calcite component to listen for. In that case, just removing the @internal label on it and restoring the original event name should suffice.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just questioning why these are internal. We may want to expose them at some point but we can do so later if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be a follow-up issue for all top-level components to publicly expose Open/Close events, or does this need to be discussed for all components, including nested?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of starting a pattern discussion on open/close events and if places that use them should expose them publicly if they aren't already. Let's create a discussion for this here: https://github.com/Esri/calcite-components/discussions

Copy link
Member

Choose a reason for hiding this comment

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

For context, these and others were made internal because it wasn't clear if they were meant to be public and there are a few that could be consolidated into a generic calcite<Event>.

src/components/dropdown-group/dropdown-group.tsx Outdated Show resolved Hide resolved
Comment on lines -363 to +368
@Event() calciteInputFocus: EventEmitter;
@Event() calciteInternalInputFocus: EventEmitter;

/**
* @internal
*/
@Event() calciteInputBlur: EventEmitter;
@Event() calciteInternalInputBlur: EventEmitter;
Copy link
Contributor

@eriklharper eriklharper May 12, 2022

Choose a reason for hiding this comment

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

I'm actually trying to remember why these two events are actually labeled @internal. These seem like public events that consumers should have access to. @driskull @jcfranco what do you all think?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the inputblur. That native event should bubble up.

The open/close ones seem like they should be public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should shadowed <input> element events bubble up when there are calciteInput* event equivalents? Seems redundant to allow both, but this is probably why these were labeled @internal in the first place because we were relying on the native events to bubble.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there can be equivalents because CustomEvents aren't the same as Keydown, Click, Blur, etc.

I think we should let the native events bubble out.

I don't like how we're passing the native events in some Custom events either. Seems kind of odd to do that.

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 I'm trying to understand the distinction between the native events and the custom events. What are the custom "Input" and "Change" events really there for if the native ones are already bubbling out and usable?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually trying to remember why these two events are actually labeled @internal.

I think I remember now. It is most likely for other Calcite components that consume Calcite Input in their shadow DOM for styling or other purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like how we're passing the native events in some Custom events either. Seems kind of odd to do that.

I guess if we chose to stop bubbling the native events, this would be the way to still expose them to listeners via the custom event.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, created a discussion for this here: https://github.com/Esri/calcite-components/discussions/4546

Copy link
Contributor

@eriklharper eriklharper May 13, 2022

Choose a reason for hiding this comment

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

I don't think there can be equivalents because CustomEvents aren't the same as Keydown, Click, Blur, etc.

Should we then ditch redundant custom events like calciteInputBlur and calciteInputFocus (and potentially many others) and just rely on native bubbled events?

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually trying to remember why these two events are actually labeled @internal.

From #1705:

Note: focus/blur events were made internal to allow for more general calciteFocus/calciteBlur events.

@Elijbet Elijbet merged commit 9f6c273 into master May 12, 2022
@Elijbet Elijbet deleted the elijbet/2256-use-calciteInternal-prefix-for-internal-events branch May 12, 2022 23:27
@github-actions github-actions bot added this to the Sprint 05/09 - 05/20 milestone May 12, 2022
@gpbmike
Copy link
Contributor

gpbmike commented Jul 25, 2022

Just leaving a note, I think this change should have been called out in the breaking changes section of the release notes.

I was using calciteInputFocus and calciteInputBlur. 😢

@jcfranco
Copy link
Member

Those props were marked as internal and omitted from the doc for quite a while now, hence why we didn't mark them as breaking. On the other hand, I understand the need for these events and will look into this in upcoming sprints (see #4611 (comment)).

@gpbmike
Copy link
Contributor

gpbmike commented Jul 25, 2022

Those props were marked as internal and omitted from the doc for quite a while now

Fair enough. I'm working on upgrading from beta.43 to beta.86. Not sure if the internal tag was there before, if so apologies. Thanks.

@jcfranco jcfranco mentioned this pull request Nov 19, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants