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

Resizable columns #1759

Merged

Conversation

brueckner
Copy link
Contributor

Hi! So this is the new counterpart to a Pull Request originally made in nextcloud/mail#4123.

It was suggested to move the implemention into the global component, so that other apps are able to use this as well. Here are some details about the implementation:

This is (still) using the splitpanes package to do the heavy lifting. We've got two columns: main and aside. These two columns have min, max and default size widths defined here.

splitpanes fires an event called resized after a resize has occurred, which triggers a save to localStorage with the size values for main and aside in onPaneResize(). The pane sizes are scoped to the parent component, which means that each app implementing this will store their own sizes automatically. To do that, I am pulling the parent component's name (this.$parent.$options.name) to construct a paneIdentifier which is then used to store and read from localStorage.

So for example, the Mail app implements this insde of the MailboxThread component. Thus the record in localStorage will look something like this:

pane-sizes.MailboxThread: {"aside":"23.60","main":"76.40"}

As for the DOM structure: the <slot> inside <main id="app-content-vue"> has now been wrapped inside <div id="app-content-wrapper">, which had to be moved up from within the component of the Mail app. This is something that needs to be checked I guess. Does any other app use that #app-content-wrapper div?

Apps that implement this component and just use the default slot should not be affected by this change, other than the new wrapping div. Maybe we could even move the default <slot> out of div#app-content-wrapper again... 🤔 That way there wouldn't be any change at all to a component that is not using the named slots. I'd love some feedback on that!

Oh and also: can someone please check why there are so many changes/additions to the package-lock.json file? Just adding the splitpanes-package shouldn't create that big of a difference I believe. 🤷‍♂️

@brueckner brueckner mentioned this pull request Mar 12, 2021
1 task
@st3iny st3iny added 3. to review Waiting for reviews enhancement New feature or request labels Mar 12, 2021
@st3iny
Copy link
Contributor

st3iny commented Mar 12, 2021

First of all, I'm really looking forward to getting this one merged :)

Oh and also: can someone please check why there are so many changes/additions to the package-lock.json file? Just adding the splitpanes-package shouldn't create that big of a difference I believe. man_shrugging

Seems like you accidentally upgraded the lock file to version 2. There are 2 options for fixing:

  1. Use npm <= v6 (lock file version 2 is used in >= v7), recreate and push the lock again.
  2. We upgrade the lock file of the repository to v2 (similar to what we did in mail not long ago).

@brueckner brueckner force-pushed the enhancement/resizable-columns branch from 43596e5 to 009dbd1 Compare March 12, 2021 08:06
@brueckner
Copy link
Contributor Author

Ha - that was it, thanks man! :) My npm was on version v7.5.4. Switching to 6xhas solved it. 👌

Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested, works well and the changes make sense

Copy link
Contributor

@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.

Thanks a lot for addressing this :)
I added some important comments 🚀

src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added feature: app-content Related to the app-content component feature: app-content-list Related to the app-content-list component labels Mar 15, 2021
@brueckner
Copy link
Contributor Author

Thanks a lot for addressing this :)
I added some important comments 🚀

Thanks for the review & feedback John @skjnldsv, I'll get on it tomorrow! :)

@brueckner brueckner force-pushed the enhancement/resizable-columns branch from 009dbd1 to 1a6784b Compare March 22, 2021 09:24
@brueckner
Copy link
Contributor Author

A little later than expected, but today I was able to implement your suggestions @skjnldsv.
Could you please have another look at it?

Cheers!

src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved
src/components/AppContent/AppContent.vue Show resolved Hide resolved
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 22, 2021
@skjnldsv
Copy link
Contributor

Getting close @brueckner ! Thank you so much! It looks great! 🤗

@brueckner brueckner force-pushed the enhancement/resizable-columns branch from 1a6784b to 39f7312 Compare March 24, 2021 16:50
@brueckner
Copy link
Contributor Author

Alright - the new update changes a few things:

Slots

There's a default slot and a list slot. If the list slot is not used, the component will behave just like before. If the list slot is used, the component will initialize the resizable columns feature.

Overriding defaults

I refactored the default settings used by the resizable columns. They can now be overriden by the parent component with 3 simple props:

<AppContent
    list-size="35"          # default: 25
    list-min-width="20"     # default: 15
    list-max-width="45"     # default: 40
>...</AppContent>

The inverse defaults for the details column are calculated automatically.

Documentation

Has been added with two examples and a section about overriding defaults.


Thanks a ton @skjnldsv, this is now much nicer to use because of your input!

If there's anything else, just let me know!

Copy link
Contributor

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

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

@raimund-schluessler
Copy link
Contributor

Will this also allow to resize the AppSidebar? This would be very neat as well and even more apps would benefit from this. 🙂

@skjnldsv
Copy link
Contributor

skjnldsv commented Apr 7, 2021

Will this also allow to resize the AppSidebar? This would be very neat as well and even more apps would benefit from this.

Nope. Might come later™ ?

@skjnldsv skjnldsv added this to the 4.0.0 milestone Apr 27, 2021
@skjnldsv
Copy link
Contributor

Hey @brueckner! Did you have a bit of time to look at the other comments? :)

@brueckner
Copy link
Contributor Author

Hey @brueckner! Did you have a bit of time to look at the other comments? :)

Oh, damn - yes and no. I have seen it, but have been distracted by other things and then forgot about it. I'll reply in the thread of @raimund-schluessler's comment. Thanks for the heads-up!

@brueckner brueckner force-pushed the enhancement/resizable-columns branch 2 times, most recently from 2fa1863 to a3810c4 Compare May 7, 2021 06:45
@brueckner
Copy link
Contributor Author

I made a little update (see #1759 (comment)) which should not break the docs on netlify anymore. I also made a reabase on master with no conflicts to solve. 👌

@skjnldsv
Copy link
Contributor

skjnldsv commented Jun 1, 2021

I'm gonna get back on it this week

I'm on it, this has become quite urgent :)

@skjnldsv
Copy link
Contributor

skjnldsv commented Jun 3, 2021

Done! I fixed some of the vue logic and implemented a very important toggle between list and details for mobile view.

	<AppContent :show-details="showDetails" @update:showDetails="hideDetails">
		<!-- contacts list -->
		<template #list>
			<ContactsList
				:list="contactsList"
				:contacts="contacts"
				:search-query="searchQuery" />
		</template>

		<!-- main contacts details -->
		<ContactDetails :contact-key="selectedContact" />
	</AppContent>

Here showDetails is a computed that is true when an item of the list is selected
hideDetails just unselect the item of the list (e.g. a contact)

@skjnldsv
Copy link
Contributor

skjnldsv commented Jun 3, 2021

Peek 03-06-2021 12-14

@skjnldsv

This comment has been minimized.

@raimund-schluessler

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@brueckner

This comment has been minimized.

Signed-off-by: Johannes Brückner <johannes@dotplex.com>
@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv force-pushed the enhancement/resizable-columns branch from b138cc7 to 2c6719a Compare June 3, 2021 12:19
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the enhancement/resizable-columns branch from 2c6719a to a611cf6 Compare June 3, 2021 12:21
@skjnldsv
Copy link
Contributor

skjnldsv commented Jun 3, 2021

@raimund-schluessler tasks seems ok now
Peek 03-06-2021 18-41

…Mobile

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the enhancement/resizable-columns branch from 55eb159 to 6fb785c Compare June 3, 2021 16:45
@raimund-schluessler
Copy link
Contributor

@skjnldsv Seems to work well for Tasks now (I just checked that it doesn't break the app, I didn't check the functionality of the resizable columns, though).

@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 4, 2021
@skjnldsv skjnldsv merged commit a9e05f2 into nextcloud-libraries:master Jun 4, 2021
@skjnldsv skjnldsv deleted the enhancement/resizable-columns branch June 4, 2021 12:50
@skjnldsv skjnldsv mentioned this pull request Jun 4, 2021
skjnldsv added a commit that referenced this pull request Jun 4, 2021
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
skjnldsv added a commit that referenced this pull request Jun 4, 2021
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 breaking PR that requires a new major version enhancement New feature or request feature: app-content Related to the app-content component feature: app-content-list Related to the app-content-list component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants