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

Replace ember-context #70

Merged
merged 5 commits into from
Jan 11, 2024

Conversation

marianoggf
Copy link
Contributor

@marianoggf marianoggf commented Jan 11, 2024

This PR moves the logic ember-context to this addon because, as a dependency, it causes issues with CI (see Slack thread for more details).

Removing the concept of that library in favor of sending data from ancestor components directly down to descendant components was too much effort, and creating something to avoid that also seemed too time-consuming. So, I went with this approach that will allow us to move forward with updating Ember 4, and we can discuss a more adequate solution later on.

The chosen place within the directory of the app was insidecomponents/polaris-resource-list because it was the only component creating the context (and the components within). It can be definitely moved to a more generic place if that is preferred.

Please share your thoughts.

@marianoggf marianoggf self-assigned this Jan 11, 2024
@marianoggf marianoggf marked this pull request as ready for review January 11, 2024 18:17
@marianoggf marianoggf requested a review from a team as a code owner January 11, 2024 18:17
@marianoggf marianoggf requested review from nahrivera7 and removed request for a team January 11, 2024 18:17
Copy link
Contributor

@nahrivera7 nahrivera7 left a comment

Choose a reason for hiding this comment

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

LGTM

@marianoggf marianoggf merged commit 6d8c6da into address-ember-4-deprecations-part-2 Jan 11, 2024
9 checks passed
@marianoggf marianoggf deleted the replace-ember-context branch January 11, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants