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

Link panel styling improvements #2917

Merged
merged 15 commits into from
Jan 29, 2020
Merged

Link panel styling improvements #2917

merged 15 commits into from
Jan 29, 2020

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Jan 26, 2020

Description

Remaining adjustments for the links sidebar from the linked issue.

Related Issue

Motivation and Context

Make links sidebar UI fully consistent with collaborators sidebar UI.

How Has This Been Tested?

Manually with chrome + firefox and acceptance tests in docker selenium. Stumbled upon some more reasonable test scenarios - will raise separate issues for adding those tests.

Screenshots (if appropriate):

Bildschirmfoto 2020-01-26 um 17 00 04

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@PVince81
Copy link
Contributor

Went through the commits with @kulmann and it looks good so far.

Next up is updating the tests.

@kulmann kulmann added Status:Needs-Review Needs review from a maintainer and removed Status:In-Progress labels Jan 28, 2020
@PVince81 PVince81 changed the title WIP Link panel tweaks Link panel styling improvements Jan 28, 2020
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Contributor

only one fail related to the iPhone suite, liked needs #2927

@kulmann
Copy link
Member Author

kulmann commented Jan 29, 2020

only one fail related to the iPhone suite, liked needs #2927

Yep, probably. I ran the test locally multiple times, sometimes successful, sometimes failing. And restarted the drone job two times, all of them failed. 😁

@PVince81
Copy link
Contributor

Rebased to get the avatar fix to get the iPhone test passing

@PVince81
Copy link
Contributor

Rebased and solved conflicts.

@kulmann while testing I found this issue and fixed it: b0cfc83
To see it you need to click on the anchors in the file list to switch to another file while keeping the links tab visible.

@PVince81 PVince81 mentioned this pull request Jan 29, 2020
15 tasks
@PVince81 PVince81 merged commit c44395a into master Jan 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the link-panel-tweaks branch January 29, 2020 16:41
@kulmann
Copy link
Member Author

kulmann commented Jan 30, 2020

Rebased and solved conflicts.

Nice, thanks!

@kulmann while testing I found this issue and fixed it: b0cfc83
To see it you need to click on the anchors in the file list to switch to another file while keeping the links tab visible.

That destroyed the animation again. ;-) I intentionally removed the v-else so that the panel stays visible, even when loading links. To solve the issue you found, it's a better idea to add a key on the template-line (changed to div, because keys are not allowed on virtual elements) and use the id of the highlighted file in the key. This way the panel get's re-rendered every time the highlighted file changes, but stays visible (also for animations) when links in store are changed without switching over to another file. Separate PR incoming soon.

@PVince81
Copy link
Contributor

@kulmann whatever the solution is, please make sure that the list hides itself while the progress bar happens when switching between files. It doesn't look right when the content stays visible when switching between files and clicking the anchor share indicator.

@PVince81
Copy link
Contributor

you might want to check the collaborators panel as well as I believe I've seen the v-else there as well, so maybe the animation thing is not aligned in both views

@kulmann
Copy link
Member Author

kulmann commented Jan 30, 2020

Ok, in that case I won't remove the v-else. Did some other template improvements though and made a tiny PR about it. As the transitions for removing/adding list items for both links and collaborators remain broken now, we need to discuss this. Looking into it I think that we are overusing the mutation on LINKS_LOADING. There is a difference between loading links for a different file/folder and loading links from adding/removing links on the currently highlighted file/folder. The latter should be a separate status in the store, or - my preference - even none at all, because the UI will show an animation on removal/creation anyway.

Opened an issue for discussion: #2937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants