Skip to content

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 3, 2020

Long time needed! Before it gets too messy, let's do this!

  • This move the Vue requirement and allow any javascript to register a Tab. With this PR we can now have React Tabs for example! 🚀
  • The migration of existing tabs isn't too complicated (see Sharing Tab)
  • This also remove any dependency between the Sidebar and the Tab, before any library used my the Tab needed to be registered at the Sidebar level. Making it absolutely counter-intuitive and badly implemented.

Capture d’écran_2020-10-04_01-24-35
Capture d’écran_2020-10-04_01-25-09


	/**
	 * Create a new tab instance
	 *
	 * @param {Object} options destructuring object
	 * @param {string} options.id the unique id of this tab
	 * @param {string} options.name the translated tab name
	 * @param {string} options.icon the vue component
	 * @param {Function} options.mount function to mount the tab
	 * @param {Function} options.update function to update the tab
	 * @param {Function} options.destroy function to destroy the tab
	 * @param {Function} [options.enabled] define conditions whether this tab is active. Must returns a boolean
	 */
	constructor({ id, name, icon, mount, update, destroy, enabled } = {}) {

@skjnldsv skjnldsv added 3. to review Waiting for reviews feature: sharing technical debt feature: file sidebar Related to the file sidebar component labels Oct 3, 2020
@skjnldsv skjnldsv added this to the Nextcloud 21 milestone Oct 3, 2020
@skjnldsv skjnldsv self-assigned this Oct 3, 2020
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 3, 2020

With that we should move the comments and version tabs tomorrow @rullzer what do you think? :)

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 4, 2020
@skjnldsv skjnldsv force-pushed the feat/files-sidebar-cleanup-standards branch 2 times, most recently from e0346ae to df5bae2 Compare October 4, 2020 11:52
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 4, 2020
@nextcloud nextcloud deleted a comment from faily-bot bot Oct 4, 2020
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2020

/compile amend /

@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the feat/files-sidebar-cleanup-standards branch from df5bae2 to 4fafd65 Compare October 5, 2020 10:09
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

sure

@skjnldsv skjnldsv force-pushed the feat/files-sidebar-cleanup-standards branch from 4fafd65 to 82952e8 Compare October 5, 2020 13:03
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2020

Fixed jsunit

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Oct 5, 2020
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2020

Missing reviewer, I thought I saw 2 🤷

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 5, 2020

Failures unrelated, waiting for last two acceptances tests!
Thanks @danxuliu for the talk pr already! 🚀

@danxuliu
Copy link
Member

danxuliu commented Oct 5, 2020

Thanks @danxuliu for the talk pr already! 🚀

My pleasure :-)

I found an issue, though, but I have not verified yet why it happens, so it may be a bug here or in the changes I made for Talk:

The header for the Chat tab will be the active one, but the contents of the tab will show the Sharing contents.

As I said it could very well be a bug in the Talk implementation, so it could be good to disable Talk and instead register a dummy tab which is shown before the Sharing tab to test this.

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

The issue seems to be in this pull request:

  • Disable comments and versions, as they have not been ported yet; also disable Talk if it was enabled from a previous test
  • Add the following code in apps/files_sharing/src/files_sharing_tab.js before registering the Sharing tab so two dummy tabs are created:
		OCA.Files.Sidebar.registerTab(new OCA.Files.Sidebar.Tab({
			id: 'aaa',
			name: 'Aaa',
			icon: 'icon-file',

			async mount(el, fileInfo, context) {
				const paragraph = document.createElement('p')
				const text = document.createTextNode('Hello world')
				paragraph.append(text)
				el.append(paragraph)
			},
			update(fileInfo) {
			},
			destroy() {
			},
		}))
		OCA.Files.Sidebar.registerTab(new OCA.Files.Sidebar.Tab({
			id: 'aaa2',
			name: 'Aaa2',
			icon: 'icon-file',

			async mount(el, fileInfo, context) {
				const paragraph = document.createElement('p')
				const text = document.createTextNode('Greetings world')
				paragraph.append(text)
				el.append(paragraph)
			},
			update(fileInfo) {
			},
			destroy() {
			},
		}))
  • Rebuild JavaScript files
  • In the Files app, create a file
  • Open the File menu actions of the file and click Details, so the sidebar opens on the first tab

The header for the Aaa tab will be the active one, but the contents of the tab will show the Aaa, Aaa2 and Sharing contents (they might be vertically out of screen due to the min-height: 100% of the contents, check the scrollbar). When another tab is clicked the previous tab contents are hidden, although all those not clicked yet are still shown. Inspecting the DOM it can be seen that after the sidebar was originally opened all the sections were visible, but after selecting a specific tab the previous section style was changed to display: none.

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 6, 2020

The header for the Aaa tab will be the active one, but the contents of the tab will show the Aaa, Aaa2 and Sharing contents (they might be vertically out of screen due to the min-height: 100% of the contents, check the scrollbar). When another tab is clicked the previous tab contents are hidden, although all those not clicked yet are still shown. Inspecting the DOM it can be seen that after the sidebar was originally opened all the sections were visible, but after selecting a specific tab the previous section style was changed to display: none.

Yes, this is fixed in upstream vue components.
Sorry, forgot about that nextcloud-libraries/nextcloud-vue#1429

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 6, 2020

  • Disable comments and versions, as they have not been ported yet; also disable Talk if it was enabled from a previous test

Also, you can keep them, they are fully compatible with this new API btw :)

@danxuliu
Copy link
Member

danxuliu commented Oct 6, 2020

Yes, this is fixed in upstream vue components.

Cool :-) Should we wait for a new nextcloud-vue release and bump it in this pull request before merging?

Also, you can keep them, they are fully compatible with this new API btw :)

Ah, sorry, I got confused by a previous comment. Great :-)

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Tested (with nextcloud-libraries/nextcloud-vue#1429) and works 👍

But please do not merge before @skjnldsv had time to answer about the nextcloud-vue bump ;-)

@skjnldsv

This comment has been minimized.

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 7, 2020

/compile amend /

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 7, 2020
@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the feat/files-sidebar-cleanup-standards branch from 5e7970d to 092103d Compare October 7, 2020 07:19
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@skjnldsv skjnldsv force-pushed the feat/files-sidebar-cleanup-standards branch from 092103d to 185f844 Compare October 7, 2020 07:41
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 7, 2020

/compile amend /

@nextcloud nextcloud deleted a comment from faily-bot bot Oct 7, 2020
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the feat/files-sidebar-cleanup-standards branch from 185f844 to eca4682 Compare October 7, 2020 07:53
@skjnldsv skjnldsv merged commit fd178b9 into master Oct 7, 2020
@skjnldsv skjnldsv deleted the feat/files-sidebar-cleanup-standards branch October 7, 2020 09:13
@PVince81
Copy link
Member

Regression with file renames in share sidebar (and possibly others): #24101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish feature: file sidebar Related to the file sidebar component feature: sharing technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants