Migrate ProgressModal to KDS#2968
Conversation
To make migration to KModal simpler - Remove props that are not used from anywhere - Remove logic related to copying or moving as `ProgressModal` is not used for these operations anymore - Remove `showProgressModal` data that is used only in v-model of `ProgressModal`. `ProgressModal` doesn't use it at all to decide if it should be displayed and calculates these criteria internally instead.
...curation/contentcuration/frontend/channelEdit/views/progress/__tests__/progressModal.spec.js
Show resolved
Hide resolved
...curation/contentcuration/frontend/channelEdit/views/progress/__tests__/progressModal.spec.js
Outdated
Show resolved
Hide resolved
| <template slot="actions"> | ||
| <KButton | ||
| v-if="isFinished || currentTaskError" | ||
| primary | ||
| data-test="refresh-button" | ||
| @click="closeOverlay" | ||
| > | ||
| {{ $tr('refreshButton') }} | ||
| </KButton> | ||
| <KButton | ||
| v-else | ||
| primary | ||
| data-test="stop-button" | ||
| @click="displayCancelModal = true" | ||
| > | ||
| {{ $tr('stopButton') }} | ||
| </KButton> | ||
| </template> |
There was a problem hiding this comment.
We should favor using the props for the buttons (IMO) unless there is a stylistic need not to because that will guarantee the best consistency in styling.
If the only reason you've opted to use action slot here is because you need those data-test attrs then I think that's a code smell that we should address (although not here probably).
If we need such granular tests that we are testing that a button exists - then that test should live in KDS on the KModal component because it's the component making sure that the button exists. We should instead only write tests for the things we control - which is the value of the props (or actions) that we pass into KModal (which is what you're testing in this case to be sure - but I'm making this comment with the idea in mind that the props way of doing things is the ideal in this case).
I wonder, if you do this through props and then mount KModal and trigger submit or cancel events on it I wonder if you can then just test that the code you've passed to the respective props is executed?
I can't say that I know the best answer or solution - but I definitely think we need to bear in mind how easy it is to use KDS components in tests as we continue working toward writing documentation for our components.
There was a problem hiding this comment.
Reviewing this - I realize now that the buttons are v-if/v-else - so my preceding comment is less relevant here than I thought at first.
There was a problem hiding this comment.
If the only reason you've opted to use action slot here is because you need those data-test attrs then I think that's a code smell that we should address (although not here probably).
Reviewing this - I realize now that the buttons are v-if/v-else - so my preceding comment is less relevant here than I thought at first.
Yes, the slot is there because of v-ifs.
If we need such granular tests that we are testing that a button exists - then that test should live in KDS on the KModal component because it's the component making sure that the button exists.
Yes, I agree.
This is not the purpose of this test suite though. It rather tests that when you click the button, a corresponding action will be triggered which has nothing to do with KModal itself. If I test for the presence of buttons, it's just to ensure that I can click them or they are buttons that are conditionally rendered, and the decision is a responsibility that belongs to ProgressModal.
I wonder, if you do this through props and then mount KModal and trigger submit or cancel events on it I wonder if you can then just test that the code you've passed to the respective props is executed?
The following comment is a more general comment and I am not sure if it's relevant to what you meant by this as it looks to be related to a specific issue that you've described. As you mentioned on Slack, this can serve as a discussion for our further work.
I would have two options how to test this example scenario: "When I click confirm stop button on the cancel modal, a certain action will be triggered (e.g. stop publishing)"
(1) Find KModal and trigger 'cancel' event
(2) Find a cancel button and trigger 'click'
I prefer (2) because that's what the user is doing (I've tried to explain this a bit in this comment) and if we change some internals, e.g. swap the modal component for something else or even just rename some of its props or events, the test won't break. I would not expect tests to break as long as the user-facing interface is the same. In this PR, going with (1) wouldn't be such a big deal, though again, generally speaking, this approach very often causes extra maintenance work and sometimes gives bad results. Also, I understand the test suite to be a documentation of the component, and if its description says "When I click confirm stop button", that's what I would expect the test to do.
That said, in some cases, reaching into internals may make sense or may even be unavoidable. There is no perfect solution. We can chat about this more.
but I definitely think we need to bear in mind how easy it is to use KDS components in tests as we continue working toward writing documentation for our components.
Happy to chat about it too. In this PR, I haven't had any problems with KModal, I could easily reach for a button in this way. Even if the name attribute wasn't present, I could just find a second button, use a class, etc.
nucleogenesis
left a comment
There was a problem hiding this comment.
Overall the KDS implementation looks great! Tested it briefly and had no issues
| <template slot="actions"> | ||
| <KButton | ||
| v-if="isFinished || currentTaskError" | ||
| primary | ||
| data-test="refresh-button" | ||
| @click="closeOverlay" | ||
| > | ||
| {{ $tr('refreshButton') }} | ||
| </KButton> | ||
| <KButton | ||
| v-else | ||
| primary | ||
| data-test="stop-button" | ||
| @click="displayCancelModal = true" | ||
| > | ||
| {{ $tr('stopButton') }} | ||
| </KButton> | ||
| </template> |
There was a problem hiding this comment.
Reviewing this - I realize now that the buttons are v-if/v-else - so my preceding comment is less relevant here than I thought at first.
Description
KModalinstead ofVDialoginProgressModalAt first, I tried to only swap
VDialogandKModalin this component. However, in this case, it seemed to be easier to switch to the KDS approach completely, for the whole component, especially due to differences related to the actions buttons slot.The first commit contains a small cleanup that has made all my further work easier. See the commit message for explanations.
Issue Addressed
One part of learningequality/kolibri-design-system#156
Before/After Screenshots
Publish
Sync
Steps to Test
Does this introduce any tech-debt items?
By this work, we've lost some transition animations between the progress window and the cancel window. I've decided not to implement it because, at this point, our transitions, in general, seem to be rather random than consistent (compare transitions in the first two steps of sync workflow to publish workflow, for example).
Checklist
Comments
Some relevant issues (follow-ups or current problems with tasks):
nullprogress value in a task metadata #2953