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

Add version naming UI #35080

Closed

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Nov 10, 2022

⚠️ ⚠️
New elements are not visible by default. Need to set OC.experimental = true in the console before opening the sidebar.
⚠️ ⚠️

Context Screenshot
List of version with the actions menu opened for the current version Screenshot from 2022-11-17 17-07-56
List of version with the actions menu opened for a previous version Screenshot from 2022-11-17 17-07-46
Name editor when the version do have a name Screenshot from 2022-11-16 14-52-17
Name editor when the version does not have a name Screenshot from 2022-11-16 14-52-08

Need #34769

@artonge artonge added the 2. developing Work in progress label Nov 10, 2022
@artonge artonge added this to the Nextcloud 26 milestone Nov 10, 2022
@artonge artonge self-assigned this Nov 10, 2022
@artonge
Copy link
Contributor Author

artonge commented Nov 10, 2022

@jancborchardt or @nimishavijay for early design review :)

  • I did not add a "Cancel" button" in the name version modal, as it seems like we do not do that in other place.
  • Should I add a "Create new version" at the top of the list or something ?
  • Let me know if I missed another requirement

@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 force-pushed the artonge/feat/version_naming_ui branch from de06c86 to 461a65e Compare November 14, 2022 13:28
@artonge artonge mentioned this pull request Nov 14, 2022
10 tasks
@artonge artonge force-pushed the artonge/feat/version_naming_ui branch 2 times, most recently from 87d69f0 to 3c0ff2c Compare November 14, 2022 14:35
@artonge artonge marked this pull request as ready for review November 14, 2022 14:37
@artonge artonge added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 14, 2022
@artonge artonge mentioned this pull request Nov 14, 2022
23 tasks
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very nice! :) Some design feedback:

  • The pencil icon for "Name this version" is a bit complex, just use the regular "pencil" material design icon
  • "Restore version" can go before "Download version" in the menu
  • The modal needs a heading inside, saying "Version name"
  • The "Named versions are persisted …" info text should go below the input field, and be in color-text-maxcontrast
  • Is the "Version name" input field focused by default?
  • If there is no version name or if the field is empty, the "Remove version name" button can be disabled?
  • Let’s use a better icon than the floppy for saving :D Just the "done" checkmark is fine

I did not add a "Cancel" button" in the name version modal, as it seems like we do not do that in other place.

I’d say an "x" in the top right would be nice though. :)

Should I add a "Create new version" at the top of the list or something ?

I’d say no, since the current version is already there? You can name the current version, right? So that is enough.

Great work @artonge! :)

@artonge
Copy link
Contributor Author

artonge commented Nov 16, 2022

I’d say no, since the current version is already there? You can name the current version, right? So that is enough.

Now you can :)

@artonge artonge force-pushed the artonge/feat/version_naming_ui branch 2 times, most recently from ef378f4 to 22b5f09 Compare November 16, 2022 14:29
@jancborchardt
Copy link
Member

Very very nice @artonge! One last thing: Both "Current version" and "Initial version" i'd consider "virtual" names, not actual version names, so I'd label the action "Name this version" and have an empty input field in the modal. (But if the name is left empty for either the initial or current version, ofc this "virtual" name stays displayed in the version list.)

Does that make sense? :D

@artonge
Copy link
Contributor Author

artonge commented Nov 16, 2022

Both "Current version" and "Initial version" i'd consider "virtual" names

"Initial version" is a "random" name I set myself for testing. Do you want to have this "virtual" label for the first version as well ?

The other part makes sense :)

  • Better handling of virtual label for the first and current versions

Edit: I've implemented the "Initial version" thing.

@artonge artonge force-pushed the artonge/feat/version_naming_ui branch from 22b5f09 to 29415e4 Compare November 17, 2022 15:39
@artonge artonge requested review from a team, PVince81 and skjnldsv and removed request for a team November 17, 2022 15:40
@artonge artonge requested a review from szaimen November 17, 2022 15:40
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

just tested a bit and how can I set the version? Also I have two initial versions?
image

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/feat/version_naming_ui branch from 29415e4 to 551c83c Compare November 17, 2022 16:06
@artonge
Copy link
Contributor Author

artonge commented Nov 17, 2022

Ah, forgot to properly set the condition for initial version.
New buttons are not visible by default. You need to set OC.experimental = true in the console before opening the sidebar.
I just pushed a fix

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

okay found it. A few more findings:

  • showing the menu dots and/or icons only on hover does not work well on mobile
  • I can use the same name for multiple items?

image

@artonge
Copy link
Contributor Author

artonge commented Nov 17, 2022

  • showing the menu dots and/or icons only on hover does not work well on mobile

That should probably be fixed in nc-vue, no ?

  • I can use the same name for multiple items?

Yes, unless @jancborchardt says otherwise :)

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Also, shouldnt the save version name button not only be enabled after you type in something?
image

@artonge
Copy link
Contributor Author

artonge commented Nov 17, 2022

Also, shouldnt the save version name button not only be enabled after you type in something?

This is so the user can remove the name. From Jan:

If there is no version name or if the field is empty, the "Remove version name" button can be disabled?

If we disable "Remove version name" then we can't also disable "Save version name".

@XueSheng-GIT
Copy link

@artonge thanks a lot! I really appriciate this approach to add version naming (and make such versions persistent)! I was looking for such a feature for quite some time.

Topics which might be considered:

  • version numbers (e.g. automatically increased or manually defined), personally I'm used to SharePoint and a M.N version scheme (Major, minor numbers, whereas e.g. 4.0 would be a stable/released version and 4.8 would be a draft version ahead of 5.0). This provides a quick overview about the versions.
  • descriptive text (optional) in addition to the the version name. Like you typically do for a github commit. This would be helpful to add some additional text about the version (and why it is important to keep or what was changed in comparison to previous versions).

@skjnldsv
Copy link
Member

OC.experimental = true, what is this? This is no standard we have, right?
I'm not sure it's a good idea to add.

@PVince81
Copy link
Member

@artonge in general I think feature flags, if we do them, should be done on capability level

now, if the backend is close to MVP maybe we can just merge the two PRs first and no need for a flag

@artonge
Copy link
Contributor Author

artonge commented Nov 23, 2022

Closing as it will get merged with #35160

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.

6 participants