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

[full-ci] Simplify Files-AppBar #6662

Merged
merged 16 commits into from
Mar 29, 2022
Merged

[full-ci] Simplify Files-AppBar #6662

merged 16 commits into from
Mar 29, 2022

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Mar 24, 2022

Description

The idea is to simplify the AppBar.vue component, which currently is bloated with a lot of edgecases depending on the currently active view instead of rendering based on props.
Even if it means some more duplication now (as tradeoff from a lot of conditional statements) it's a necessary step to eventually build a abstracted FilesList-layout (which would then definitely reduce duplication & complexity).

Done

  • Extract breadcrumb creation into TypeScript helper
  • Used AppBar via props & slots and move business logic to view layer
  • Moved FileDrop component to CreateAndUpload component to reduce shared logic
  • Created new EmptyTrashbin.vue component for use in Trashbin/Appbar

Related Issues

Caveats

  • "ViewOptions" is left-aligned if no create/batch actions (was the case before this PR already but can conveniently be fixed in this) (ViewOptions sometimes not correctly aligned #6685) @pascalwengerter
  • Shares-page: When switching between the shares-pages, there's a tiny visual glitch when the page gets rerendered (including appBar). Maybe we can add a SharesLayout and render pages into it to avoid it?
  • Renaming a folder via the breadcrumb-contextaction now re-renders the ResourceTable (? 🤔 )
  • ViewOptions conditional rendering is suboptimal (showing 'Toggle hidden files' on Spaces Projects overview) @pascalwengerter
  • AppBar => naming for static slot is not good => renamed to content
  • Breadcrumbs showing contextActions for root node & additional arrow-right for last node

@update-docs
Copy link

update-docs bot commented Mar 24, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@pascalwengerter pascalwengerter force-pushed the simplify-appbar branch 2 times, most recently from 74524d7 to e5b9e8a Compare March 28, 2022 15:15
@pascalwengerter pascalwengerter marked this pull request as ready for review March 28, 2022 15:15
@pascalwengerter pascalwengerter changed the title [WIP] - Simplify Files-AppBar Simplify Files-AppBar Mar 28, 2022
@pascalwengerter pascalwengerter changed the title Simplify Files-AppBar [full-ci] Simplify Files-AppBar Mar 28, 2022
@@ -51,22 +51,6 @@ describe('User can navigate in files list using pagination', () => {
Files: StoreFiles
}
})
const appBar = document.createElement('div')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly, we could get rid of this weird workaround here now 😅

@@ -37,12 +37,6 @@ describe('AppBar contains set of actions and informations', () => {
render(
AppBar,
{
setup: () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to proceed here, we'd have to either render a full view or provide breadcrumb prop items to complete the rendering and fulfill the test criteria

SideBar
},
mixins: [Mixins],
data() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea of these were actually necessary or long-forgotten leftovers?

Copy link
Member

Choose a reason for hiding this comment

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

If they are not referenced in the template ... happy they go

@ownclouders
Copy link
Contributor

ownclouders commented Mar 28, 2022

Results for oCISFiles1 https://drone.owncloud.com/owncloud/web/24236/52/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUICreateFilesFolders-createFile_feature-L13.png

webUICreateFilesFolders-createFile_feature-L13.png

webUICreateFilesFolders-createFile_feature-L19.png

webUICreateFilesFolders-createFile_feature-L19.png

webUICreateFilesFolders-createFile_feature-L25.png

webUICreateFilesFolders-createFile_feature-L25.png

webUICreateFilesFolders-createFile_feature-L31.png

webUICreateFilesFolders-createFile_feature-L31.png

webUIDeleteFilesFolders-deleteFilesFolders_feature-L253.png

webUIDeleteFilesFolders-deleteFilesFolders_feature-L253.png

webUIDeleteFilesFolders-deleteFilesFolders_feature-L77.png

webUIDeleteFilesFolders-deleteFilesFolders_feature-L77.png

webUIFilesCopy-copy_feature-L131.png

webUIFilesCopy-copy_feature-L131.png

webUIFilesCopy-copy_feature-L142.png

webUIFilesCopy-copy_feature-L142.png

webUIFilesCopy-copy_feature-L28.png

webUIFilesCopy-copy_feature-L28.png

webUIFilesCopy-copy_feature-L70.png

webUIFilesCopy-copy_feature-L70.png

webUIFilesCopy-copy_feature-L71.png

webUIFilesCopy-copy_feature-L71.png

webUIFilesCopy-copy_feature-L72.png

webUIFilesCopy-copy_feature-L72.png

webUIFilesCopy-copy_feature-L73.png

webUIFilesCopy-copy_feature-L73.png

webUIFilesCopy-copy_feature-L76.png

webUIFilesCopy-copy_feature-L76.png

webUIMarkdownEditor-markdownFile_feature-L13.png

webUIMarkdownEditor-markdownFile_feature-L13.png

webUIMarkdownEditor-markdownFile_feature-L21.png

webUIMarkdownEditor-markdownFile_feature-L21.png

webUIMarkdownEditor-markdownFile_feature-L29.png

webUIMarkdownEditor-markdownFile_feature-L29.png

webUIMarkdownEditor-markdownFile_feature-L37.png

webUIMarkdownEditor-markdownFile_feature-L37.png

webUIMarkdownEditor-markdownFile_feature-L44.png

webUIMarkdownEditor-markdownFile_feature-L44.png

webUIMarkdownEditor-markdownFile_feature-L50.png

webUIMarkdownEditor-markdownFile_feature-L50.png

webUIMarkdownEditor-markdownFile_feature-L56.png

webUIMarkdownEditor-markdownFile_feature-L56.png

webUIMarkdownEditor-markdownFile_feature-L70.png

webUIMarkdownEditor-markdownFile_feature-L70.png

webUIMarkdownEditor-markdownFile_feature-L71.png

webUIMarkdownEditor-markdownFile_feature-L71.png

webUIMarkdownEditor-markdownFile_feature-L80.png

webUIMarkdownEditor-markdownFile_feature-L80.png

webUIMarkdownEditor-markdownFile_feature-L81.png

webUIMarkdownEditor-markdownFile_feature-L81.png

webUIMarkdownEditor-markdownFile_feature-L82.png

webUIMarkdownEditor-markdownFile_feature-L82.png

webUIMarkdownEditor-markdownFile_feature-L83.png

webUIMarkdownEditor-markdownFile_feature-L83.png

webUIMarkdownEditor-markdownFile_feature-L84.png

webUIMarkdownEditor-markdownFile_feature-L84.png

@@ -0,0 +1,6 @@
Bugfix: AppBar ViewOptions alignement
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Bugfix: AppBar ViewOptions alignement
Bugfix: AppBar ViewOptions alignment

SideBar
},
mixins: [Mixins],
data() {
Copy link
Member

Choose a reason for hiding this comment

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

If they are not referenced in the template ... happy they go

Copy link
Collaborator

@fschade fschade left a comment

Choose a reason for hiding this comment

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

LGTM if ci is happy 🚀

@sonarcloud
Copy link

sonarcloud bot commented Mar 29, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

52.4% 52.4% Coverage
3.2% 3.2% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ViewOptions sometimes not correctly aligned
5 participants