-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
UI: Add poststart and poststop lifecycle phases #8742
Conversation
Ember Asset Size actionAs of 685b97a Files that got Bigger 🚨:
Files that got Smaller 🎉:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
a25c64a
to
7cf9a43
Compare
My suggestion is that this table isn’t sufficiently useful to keep around with the combinatoric explosion of other lifecycle phases. The logic was that someone might wonder “why isn’t my main task starting?” and this table would show that the prestart tasks hadn’t yet completed. One might wonder the same about any task that has prerequisites, so should a poststart task have a table that shows main tasks? And so on. Since the route hierarchy guarantees that one has already passed through a template that shows the lifecycle chart before one can reach the template where this table is displayed, I believe this table is redundant. It also conveys information in a more abstract way than the chart, which is dense and more easily understood, to me.
7cf9a43
to
0d885be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good!
I read the commentary about removing the prestart task table and I think your justification is totally reasonable.
I noticed there are starting to be some unwieldy if/else chains, but I think it's okay considering there's still a finite number of states this chart will ever express. I also don't have any concrete suggestions for how to improve it 😅
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This adds new phases for the lifecycle chart. Poststart is treated as a subphase of Main, mostly just a label to hint at the alignment of tasks in the chart. Similarly to with the prestart phase, poststart sidecar task states are ignored when determining whether Main is showing as active.
I decided to make all phases show whenever there’s a non-main task. I removed the bulk of the acceptance test because it’s better covered in the integration test.
I also decided to entirely remove the “prestart tasks” table that formerly showed when viewing a main task, with explanation in 0d885be. It or something like it could be easily brought back with discussion of how to address the verbosity etc.
Here’s an example configuration chart:
Here’s a GIF of it sort of working with a build combining #8194 and #8390, though poststop seems to not work as I’d expect:
I don’t love submitting things for review without having verified functionality with a true API, but I think it’s okay in this case because it’s further permutations on values for the existing lifecycle model fragment.