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

docs: account for this context #28720

Merged
merged 4 commits into from
Dec 19, 2023
Merged

docs: account for this context #28720

merged 4 commits into from
Dec 19, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Dec 18, 2023

Issue number: N/A


What is the current behavior?

In #28694 there was some confusion around how to access this inside of a callback function passed to a property on Ionic components. The root issue was due to how the this context is determined with developers being responsible for setting the appropriate this context.

What is the new behavior?

  • While this isn't an Ionic bug, I think it's worth calling out this behavior so developers are aware of how to account for it.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Note: The link in the docs will not work until ionic-team/ionic-docs#3333 is merged.

@github-actions github-actions bot added the package: core @ionic/core package label Dec 18, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review December 18, 2023 17:46
@@ -341,6 +341,10 @@ export class Datetime implements ComponentInterface {
* dates are selected. Only used if there are 0 or more than 1
* selected (i.e. unused for exactly 1). By default, the header
* text is set to "numberOfDates days".
*
* Developers who wish to access "this" inside of the function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Developers who wish to access "this" inside of the function
* Angular developers who wish to access "this" inside of the function

Should it specify the framework? My concern is that other framework users might get confused with the comment.

Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

I question if this is the best way to document this tip, for a few reasons:

  1. It creates a lot of duplicate text, which is annoying to maintain.
  2. I can see us easily forgetting to add the same tip to any future function props.
  3. It seems to be Angular-specific -- I was unable to access the right this in React (using an arrow function) or Vue (using .bind(this)). If it does actually apply to React or Vue, but my example is just subtly wrong, that only speaks to the need for example code somewhere.

Maybe we could put it in Development Tips instead? Or a page in the Angular-specific docs if it really is only relevant to Angular? I know that's not as discoverable, but my understanding is that this isn't an Ionic-specific issue, so I'm unsure if we need to give it much priority in the docs.

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@averyjohnston averyjohnston left a comment

Choose a reason for hiding this comment

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

LGTM aside from a component that was missed when updating to the link.

core/src/components/modal/modal.tsx Outdated Show resolved Hide resolved
@liamdebeasi liamdebeasi added this pull request to the merge queue Dec 19, 2023
Merged via the queue into main with commit 4cf948f Dec 19, 2023
46 checks passed
@liamdebeasi liamdebeasi deleted the formatter-context branch December 19, 2023 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants