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

Port files_versions to vue #34769

Merged
merged 9 commits into from
Nov 29, 2022
Merged

Port files_versions to vue #34769

merged 9 commits into from
Nov 29, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Oct 24, 2022

Simplify code and make it use our standard components, will be helpful when adding more features in the future

After

Screenshot from 2022-11-09 13-12-16

image

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Oct 24, 2022
@CarlSchwan CarlSchwan requested a review from a team October 24, 2022 11:32
@CarlSchwan CarlSchwan self-assigned this Oct 24, 2022
@CarlSchwan CarlSchwan requested review from PVince81, skjnldsv and szaimen and removed request for a team October 24, 2022 11:32
@CarlSchwan CarlSchwan force-pushed the port/vue/files_version branch 3 times, most recently from 6b15d21 to c2276f1 Compare October 24, 2022 11:58
@PVince81
Copy link
Member

is the border shadow around the thumbnail on purpose ? are we using this style elsewhere ?

is it on purpose that the row buttons are more blue-ish than the ones in the header ?

@CarlSchwan
Copy link
Member Author

is the border shadow around the thumbnail on purpose ? are we using this style elsewhere ?

Yeah I added them because otherwise the preview is just looking really weird for any text document (and files versions is primarely used for text documents)

image

Alternative we could add either a more subtle shadow or a simple border

@jancborchardt @nimishavijay what do you think?

is it on purpose that the row buttons are more blue-ish than the ones in the header ?

It's using secondary action type and not the ternary action type, not sure what is the appropriate here as we don't have any guidelines documented somewhere :(

apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the port/vue/files_version branch 2 times, most recently from 8d425cc to 90023c6 Compare October 24, 2022 14:21
@CarlSchwan CarlSchwan added this to the Nextcloud 26 milestone Oct 24, 2022
@jancborchardt
Copy link
Member

jancborchardt commented Oct 24, 2022

Yeah I added them because otherwise the preview is just looking really weird for any text document (and files versions is primarely used for text documents)

@CarlSchwan no problem, that's what we have border: 1px solid var(--color-border); for. ;) And remember the border radius too – just like we do for e.g. Mail attachments and the file list. (Is this a component yet?)

Drop shadow here does not really make sense as nothing is elevated. :) That is, the text linked with the preview would not have a drop shadow either.

@skjnldsv
Copy link
Member

A few eslint issues to fix :)

@CarlSchwan
Copy link
Member Author

Yeah I added them because otherwise the preview is just looking really weird for any text document (and files versions is primarely used for text documents)

@CarlSchwan no problem, that's what we have border: 1px solid var(--color-border); for. ;) And remember the border radius too – just like we do for e.g. Mail attachments and the file list. (Is this a component yet?)

Drop shadow here does not really make sense as nothing is elevated. :) That is, the text linked with the preview would not have a drop shadow either.

changed to a simple border :) and I updated the PR description with the latest visual

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Beside the logger comment, feel free to ignore the others as they are not crucial :)

apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
apps/files_versions/src/views/VersionTab.vue Outdated Show resolved Hide resolved
@skjnldsv
Copy link
Member

@artonge added comments :)
But we're close to completion! 🚀

@artonge artonge force-pushed the port/vue/files_version branch 3 times, most recently from c5a7016 to b094abd Compare November 10, 2022 16:48
@artonge artonge mentioned this pull request Nov 14, 2022
@artonge artonge mentioned this pull request Nov 23, 2022
23 tasks
CarlSchwan and others added 9 commits November 28, 2022 17:29
Simplify code and make it use our standard components

Signed-off-by: Carl Schwan <[email protected]>
Signed-off-by: Carl Schwan <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Carl Schwan <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Carl Schwan <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
@artonge
Copy link
Contributor

artonge commented Nov 28, 2022

Rebased. Will merge when CI is green

@artonge artonge merged commit a884f31 into master Nov 29, 2022
@artonge artonge deleted the port/vue/files_version branch November 29, 2022 09:20
@PVince81 PVince81 mentioned this pull request Nov 30, 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