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: remove ember-crumbly #651

Merged
merged 53 commits into from
Jun 13, 2023
Merged

Conversation

guidojw
Copy link
Member

@guidojw guidojw commented Nov 6, 2022

Summary

Replaces ember-crumbly with a custom implementation based on poteto/ember-crumbly#149.

Had to restructure the routes in order to retain the parent route breadcrumbs. Due to this restructure, the show routes could be renamed to index routes, making more sense. Also after our conversation on Slack it looked like the forum routes were working correctly as is, but this wasn't the case so these routes needed some restructuring as well, overall unifying the route & controller structure.

I noticed there was an unimplemented profile route being created in router.js, so I implemented this to redirect to your own user page. users/batch/confirm route also was unimplemented so I removed it.

Lastly, I restructured the tests to the new structure too and implemented all missing route unit tests.

Todo:

@guidojw guidojw linked an issue Nov 6, 2022 that may be closed by this pull request
1 task
@guidojw guidojw mentioned this pull request Nov 6, 2022
1 task
@guidojw guidojw force-pushed the refactor/remove-ember-crumbly branch from f0392b2 to 7e62b6d Compare November 6, 2022 19:08
@guidojw guidojw linked an issue Nov 17, 2022 that may be closed by this pull request
@guidojw guidojw force-pushed the refactor/remove-ember-crumbly branch from a94b73c to 0fa751e Compare November 21, 2022 21:51
@DrumsnChocolate
Copy link
Contributor

It seems that mail-moderations.mail-moderation.show.hbs has not yet been changed to mail-moderations.mail-moderation.index.hbs, I'll add a commit that does that. It's the only show file left, it seems

@DrumsnChocolate
Copy link
Contributor

the new structure seems to exploit something that I was not aware of. Is it true that in the following structure
image
the group.js works as a sort of 'super' route to the routes in the groups.group folder? In the sense that the edit, exit, and index routes in that folder inherit the model hook (and maybe more?) from group.js?

…s that belong to these routes, which was initially broken.
@guidojw
Copy link
Member Author

guidojw commented May 6, 2023

This was probably the case because it was merged into the main branch after this PR, I'll fix those things when the rest is reviewed

@guidojw
Copy link
Member Author

guidojw commented May 6, 2023

the new structure seems to exploit something that I was not aware of. Is it true that in the following structure image the group.js works as a sort of 'super' route to the routes in the groups.group folder? In the sense that the edit, exit, and index routes in that folder inherit the model hook (and maybe more?) from group.js?

This is true! Specifically with the model hooks (and breadcrumbs too) this is super handy

Copy link
Contributor

@DrumsnChocolate DrumsnChocolate left a comment

Choose a reason for hiding this comment

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

I like it! I haven't looked at every single file, but I have taken a relatively thorough look at most pages to ensure they still work. With PRs this size it is inevitable we will miss things, so we should expect bugs to occur, and we will pay some special attention to reports from users during the weeks after this is merged.
@guidojw since the bootstrap v5 migration has just been merged into staging, there are a lot of merge conflicts with the templates in this branch. I'm willing to tackle them, but if you prefer to do that yourself let me know.

I think this PR is well executed, and I haven't found any over-complicated pieces of code. well done!

@guidojw
Copy link
Member Author

guidojw commented May 7, 2023

Thanks! I will go through the merged commits one by one and fix the conflicts, and hopefully don't miss anything 🙂

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Jun 5, 2023

In light of the recent release of ember v5, I think we should like to get this merged relatively soon. Guido, will you manage to resolve the merge conflicts? If not, let me know and I will pick this up

@guidojw
Copy link
Member Author

guidojw commented Jun 5, 2023

In light of the recent release of ember v5, I think we should like to get this merged relatively soon. Guido, will you manage to resolve the merge conflicts? If not, let me know and I will pick this up

Agreed, I had a busy few months but managed to get my balance for personal projects back recently and can work on this soon. I think it's best to first get #758 merged because it will have a few conflicts with this, then I'll resolve all the conflicts here and we can merge this one. I can do that some evening this week.

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Jun 8, 2023

I can do that some evening this week.

I'm not sure that #758 will be merged by then. We've asked for feedback from the promotion committee on the current version of #758 via email, and there are still more redesigned pages coming. I think merging the crumbly PR first might actually prevent conflicts over the coming month. @Ellen-Wittingen do you want to weigh in on this?

I had a busy few months but managed to get my balance for personal projects back recently and can work on this soon.

Good to hear, don't stress yourself too much since this project is very much long term focused and things don't need to move super quick

@guidojw
Copy link
Member Author

guidojw commented Jun 8, 2023

I can also merge this and resolve the conflicts with #758 there since I know what has to be changed to keep it working without ember-crumbly, if you'd like that @Ellen-Wittingen

@Ellen-Wittingen
Copy link
Contributor

Ellen-Wittingen commented Jun 8, 2023

Yeah, #758 won't be merged till next week, so go ahead with merging this one first

@Ellen-Wittingen
Copy link
Contributor

I can also merge this and resolve the conflicts with #758 there since I know what has to be changed to keep it working without ember-crumbly, if you'd like that @Ellen-Wittingen

That would be great!

@guidojw
Copy link
Member Author

guidojw commented Jun 8, 2023

I can also merge this and resolve the conflicts with #758 there since I know what has to be changed to keep it working without ember-crumbly, if you'd like that @Ellen-Wittingen

That would be great!

All right, I'll let you know when that's done!

@guidojw guidojw enabled auto-merge June 13, 2023 20:45
@guidojw guidojw disabled auto-merge June 13, 2023 20:45
@guidojw guidojw enabled auto-merge June 13, 2023 20:52
@guidojw guidojw added this pull request to the merge queue Jun 13, 2023
Merged via the queue into staging with commit cd14eb8 Jun 13, 2023
@guidojw guidojw deleted the refactor/remove-ember-crumbly branch June 13, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mandate create has broken breadcrumb Replace ember-crumbly
3 participants