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

Rewrite the sidebar tab in Vue #581

Merged
merged 3 commits into from
May 10, 2021
Merged

Rewrite the sidebar tab in Vue #581

merged 3 commits into from
May 10, 2021

Conversation

artonge
Copy link
Collaborator

@artonge artonge commented Apr 22, 2021

This is a rewrite in Vue of the sidebar tab.

I used @juliushaertl/vue-richtext to render activities, and created components for activity types that needed it.

Done:

  • Rewrite activity tab to vue
  • Add jest tests
  • Add cypress tests
Before After
Screenshot from 2021-05-04 10-14-14 Screenshot from 2021-05-03 19-03-50

@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from 6d448c6 to b822f34 Compare April 22, 2021 16:20
package.json Outdated Show resolved Hide resolved
src/views/ActivityTab.vue Outdated Show resolved Hide resolved
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch 8 times, most recently from 1fe536e to f4d8369 Compare May 6, 2021 08:33
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from f4d8369 to 5c7f731 Compare May 6, 2021 08:50
@artonge artonge marked this pull request as ready for review May 6, 2021 08:54
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments :)
Looks great!

Let's ping Jan for design update, now this is the time to do that 🚀

cypress/cypress.json Outdated Show resolved Hide resolved
src/views/ActivityTab.vue Outdated Show resolved Hide resolved
src/models/ActivityModel.js Outdated Show resolved Hide resolved
src/components/richArgumentsTypes/FileRichArgument.vue Outdated Show resolved Hide resolved
src/components/richArgumentsTypes/EmailRichArgument.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added this to the Nextcloud 22 milestone May 6, 2021
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from 5c7f731 to 8a74a11 Compare May 6, 2021 09:54
@jancborchardt
Copy link
Member

Nice nice @artonge! :) 2 things from the design side:

  • The icons are a bit larger than previously, and as that also larger than the tab icons. Is this intentional? Smaller looks a bit nicer
  • The whitespace between entries was reduced – but we should have some more breathing room, actually more than before even. We try to use units on a scale of 4, so you can check if e.g. 8 or 12 or 16px of padding looks nicer.
  • This might not be something for this pure rewrite – but it would be quite nice if for the "[person] commented" activity instead of an icon, we use their avatar.

@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from 8a74a11 to 9fffa1e Compare May 6, 2021 10:10
@artonge
Copy link
Collaborator Author

artonge commented May 6, 2021

UI update:

  • The icons are a bit larger than previously, and as that also larger than the tab icons. Is this intentional? Smaller looks a bit nicer

I felt the opposite, I felt that they were to small previously, they felt "cheap" to me compared to the Sharing and Comments tab. Maybe we could match those ?

  • The whitespace between entries was reduced – but we should have some more breathing room, actually more than before even. We try to use units on a scale of 4, so you can check if e.g. 8 or 12 or 16px of padding looks nicer.

Indeed.

  • This might not be something for this pure rewrite – but it would be quite nice if for the "[person] commented" activity instead of an icon, we use their avatar.

Added an example with a comment from another user. It seems duplicate to change the icon comment to the user's avatar.

Screenshot from 2021-05-06 12-46-44

src/main.js Outdated Show resolved Hide resolved
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Woho, I made 3 attempts to migrate the normal activity view to Vue, but always failed to finish it completely.
This looks like a step into the future :)

@nickvergessen
Copy link
Member

This might not be something for this pure rewrite – but it would be quite nice if for the "[person] commented" activity instead of an icon, we use their avatar.

Please let's not do this here. This needs fixing on various levels and should be reflected via the API so it works in the mobile apps then too.

@artonge artonge force-pushed the feature/migrate_tab_to_vue branch 2 times, most recently from 8268e4b to 751d91f Compare May 6, 2021 13:38
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from 751d91f to 6e0430d Compare May 6, 2021 13:50
artonge added 2 commits May 10, 2021 10:31
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Louis Chemineau <louis@chmn.me
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch 2 times, most recently from 3fd6dd9 to acd0974 Compare May 10, 2021 09:10
@artonge artonge requested a review from skjnldsv May 10, 2021 09:47
@rullzer
Copy link
Member

rullzer commented May 10, 2021

Looking super slick @artonge
I vote to try to get it in asap. So that we can work on smaller fixes separatly. As to not make this bigger than it has to be right now.

CC: @skjnldsv

@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from acd0974 to a5ae3f7 Compare May 10, 2021 12:01
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch 2 times, most recently from a50d454 to ddd3acc Compare May 10, 2021 14:07
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch 2 times, most recently from a629006 to 645a662 Compare May 10, 2021 14:23
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the feature/migrate_tab_to_vue branch from 645a662 to 6cb4ee3 Compare May 10, 2021 14:59
@artonge artonge merged commit 4fe5aef into master May 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/migrate_tab_to_vue branch May 10, 2021 15:19
@artonge artonge mentioned this pull request Dec 5, 2022
4 tasks
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.

5 participants