show loading indicator while waiting for assignees first fetch
What does this MR do?
Fixes:
- Assignees section tiny and empty when first loaded
- "Assign to me" shown before Assignee loaded
Are there points in the code the reviewer needs to double check?
- is there a preferred way to flag the initial "don't display yet" state?
isFetchingcould be more accurately calledisInitialized - should Time Tracking be updated to do this as well?
Screenshots (if relevant)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary [ ] Documentation created/updated[ ] API support added- Tests
-
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Closes #32170
Merge request reports
Activity
assigned to @psimyn
@ClemMakesApps can you please have a look? I left a few notes in "to double check" section
mentioned in issue #32401 (moved)
- Do we want to fully render the current Assignee if set? more useful than spinner but means duplicate markup
I think we don't want to have duplicate markup, so I don't think we want to do that even though it would prevent the data to the user faster.
- May need tests. Wasn't sure where/what should be tested. Either testing a setter, or that a setter has been called. Can add an expect to an existing spec but seemed over-the-top adding a new test to assert loader is visible
Could probably create an integrated test using jasmine and karma. Have the test run on the generated fixtures and then make sure that the loading indicator is displayed before the api data is returned
- do we want semi-server-render like this? I am trying to minimize content jumping around after DOMContentLoaded
Addressed in comment above
- image spinner jumps after vue component is mounted. I don't know what to do about this short of actual server rendering, or fading between the two elements
Not sure what you mean by this. Could you elaborate?
Code seems fine but I'll defer to @filipa since it's vue related
added 1 commit
- 435d64af - show loading indicator while waiting for assignees first fetch
@ClemMakesApps I updated to be more CSS dependent. Has min height but not server-rendered spinner
updated description
if this method is better, should I update time tracking as well?
Edited by Simon KnoxNot sure what you mean by this. Could you elaborate?
When I had the duplicate
<i>tag in haml, the animation position changed when vue replaced the markup, shown here:
Not an issue anymore since removing the spinner icon from haml
Edited by Simon Knoxadded regression label
@filipa can you please let me know if this is an ok way to handle initial render, or if there's one you prefer
assigned to @filipa
assigned to @iamphill
assigned to @psimyn
I am going to agree with @iamphill, let's go ahead and add a placeholder element in the haml so that it is there until vue takes over. That's what we are doing for time tracking
thanks @iamphill and @ClemMakesApps! much simpler this way as well
assigned to @iamphill
Change looks good to me
@ClemMakesApps can you approve & merge?
assigned to @ClemMakesApps
assigned to @psimyn
assigned to @ClemMakesApps
@ClemMakesApps I forgot to reassign to you
added and removed
aria-hiddenas neededassigned to @psimyn
added 378 commits
- 8c74e7c4...e07c7551 - 377 commits from branch
master - 170052c5 - add loading spinner to sidebar assignees
- 8c74e7c4...e07c7551 - 377 commits from branch
assigned to @ClemMakesApps
assigned to @iamphill
enabled an automatic merge when the pipeline for 6b8fac9d succeeds
mentioned in commit 30462b88
mentioned in issue #32839 (closed)
mentioned in commit 19642f61
