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 cancel button for when user entry is being edited #38952

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JeaNugroho
Copy link

@JeaNugroho JeaNugroho commented Jun 23, 2023

Summary

Add a cancel button between checkmark button and three-dot button for when user entry is being edited.

Before

Screen Shot 2023-06-22 at 8 02 07 PM

After

Screen Shot 2023-06-21 at 10 40 21 PM

Potential next steps

This comment may be the next fix to make user experience more intuitive and straightforward. I personally would think that the user editing process would be like this if I were a new user to the app. I can certainly implement a way to make this happen, either in the same PR or in the next PR after this one's completed.

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Jun 23, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Jun 23, 2023
@szaimen szaimen requested review from a team, susnux and Fenn-CS and removed request for a team June 23, 2023 07:18
Copy link
Contributor

@Fenn-CS Fenn-CS left a comment

Choose a reason for hiding this comment

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

Thank you so far! Left an inline comment.

apps/settings/src/components/Users/UserRow.vue Outdated Show resolved Hide resolved
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.

Watch out as the component has been heavily modified recently, so your change might need to be adapted.

#38839

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Super nice! :) Only one tiny thing to nitpick: All the icons in a column should be center-aligned ideally. So the buttons should be moved slightly to the right, so that the close icon is nicely aligned with all the edit icons above it :)

Copy link
Contributor

@Fenn-CS Fenn-CS left a comment

Choose a reason for hiding this comment

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

This now looks ok to me however,

  • There's a review about alignment which you should address
  • Update the screenshot in the description to reflect the new changes

And we would be good to go! Thank you for your contribution :)

@susnux
Copy link
Contributor

susnux commented Jul 1, 2023

This needs to be rebased as the @artonge said it was heavily modified.
The settings are not saved when pressing the check button "done" but when you save a field. Similar to all other settings on Nextcloud which are saved on change rather than that there is a "save" button.

JeaNugroho and others added 3 commits July 3, 2023 20:40
Rebase add cancel button
Signed-off-by: JeaNugroho <jeanugroho@yahoo.com>
Omit checkmark button since it has the same functionality as the cancel button - Editing User Entry

Co-authored-by: F. E Noel Nfebe <fenn25.fn@gmail.com>
Signed-off-by: Jean Benedict Nugroho <35510082+JeaNugroho@users.noreply.github.com>
Signed-off-by: JeaNugroho <jeanugroho@yahoo.com>
@JeaNugroho JeaNugroho force-pushed the JeaNugroho-NextCloudServer/AddCancelButton-EditingUserEntry branch from 2e36db2 to d2b2fc3 Compare July 4, 2023 01:43
@JeaNugroho
Copy link
Author

Have just rebased! I'll try to fix the alignment and update the original commit with the new screenshot of the layout. And then when any of the required CI checks fail, I hope someone can provide guidance for that 😊

@Fenn-CS @susnux @artonge @nimishavijay

@JeaNugroho
Copy link
Author

@Fenn-CS @artonge @susnux
We now clearly know that we need to fix eslint and Node. I recall from a different PR I'm doing, we need to run npm run build to fix Node and run npm run lint:fix to fix eslint. So I don't mess this PR like the other one, in what sequence do I have to run these? And are there any other commands or things should I do before, after, or in between these two commands?

@Fenn-CS
Copy link
Contributor

Fenn-CS commented Jul 4, 2023

@JeaNugroho you should lint and then build ideally!

However there are some merge conflicts in your PR, check the files tab closely. I would leave some inline comments to help you see them.

<div class="icon-checkmark" />
{{ feedbackMessage }}
</div>
>>>>>>> 7a2938b01f3 (Add cancel button for when user entry is being edited)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
>>>>>>> 7a2938b01f3 (Add cancel button for when user entry is being edited)

Merge conflict, and deleting this line is not the solution though you want to work with perhaps your code editor to help you resolve the conflicts.

@@ -221,10 +221,36 @@
</div>

<div class="userActions">
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<<<<<<< HEAD

Another conflict marker

<UserRowActions v-if="!loading.all"
:actions="userActions"
:edit="true"
@update:edit="toggleEdit" />
=======
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
=======

Conflict.

Signed-off-by: JeaNugroho <jeanugroho@yahoo.com>
@Pytal
Copy link
Member

Pytal commented Jul 6, 2023

Thanks @JeaNugroho! We have some more modifications incoming, would it be fine with you if we revisit this after merging #39050 to prevent conflicts?

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@joshtrichards
Copy link
Member

#39050 has been merged, @JeaNugroho. So feel free to proceed. :)

This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a Cancel button when editing a user
10 participants