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

Update to glimmer components #1120

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

miguelcobain
Copy link
Collaborator

@miguelcobain miguelcobain commented Feb 9, 2022

Let's use this PR to buffer the changes to update the components (or should we merge them as they come?).

Currently migrated DocsDemo (and sub-components), DocsSnippet and DocsCodeHighlight.
Also started a dedicated docs page to the v5 breaking changes/upgrade instructions.

@SergeAstapov
Copy link
Collaborator

Thank you @miguelcobain!

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

This looks awesome, thanks @miguelcobain! Should we go ahead and merge this?

@miguelcobain
Copy link
Collaborator Author

@rwwagner90 I think merging this to master might be a bit imprudent.
This PR contains breaking changes, so merging it to master means that we wouldn't be able to easily make patch releases while this update is made.

I think merging this to a v5 branch or something similar could be a good middle ground.

@RobbieTheWagner
Copy link
Member

@miguelcobain I'm not worried about introducing breaking changes to master. I don't plan on any patch releases before releasing a new major version. I just wanted to double check with you that this was ready to go, since it sounded like you weren't sure if you wanted to do one PR for all glimmer conversions or a few.

@miguelcobain
Copy link
Collaborator Author

@rwwagner90 this PR should be completely functional. The migrated components do work and the all docs and tests were updated according to that migration. No holes here.

I think separate PRs makes it easier for you guys to review. But I can go either way. Whatever you prefer.

@RobbieTheWagner RobbieTheWagner merged commit 743f59f into ember-learn:master Feb 9, 2022
@RobbieTheWagner
Copy link
Member

@miguelcobain I went ahead and merged, thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants