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

custom profile field: Remove order column in custom-field-choices. #10160

Closed
wants to merge 3 commits into from

Conversation

YJDave
Copy link
Collaborator

@YJDave YJDave commented Aug 2, 2018

This is removes order column in choices and add drag & drop type of rows for ordering.
Fixes #10129

GIFs or Screenshots:
Before:
screen shot 2018-08-03 at 1 21 59 am
After:
screen shot 2018-06-12 at 9 33 11 am

cp_10129

@zulipbot
Copy link
Member

zulipbot commented Aug 2, 2018

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

@@ -23,7 +23,7 @@
</li>
</ul>

<table class="table table-condensed table-striped admin_filters_table">
<table class="table table-condensed admin_filters_table">
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Why this change? Should be a separate commit regardless, but I think this looks nicer this way here:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it was overriding style margin-top: 20px so removed that class.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Cool, a few thoughts on this:
(1) You should read about CSS precedence; there's a better way to ensure the CSS gets used.
(2) Generally, this sort of refactor to another component should go in its own commit, both to make it obvious you're changing another component, and also so there's a place for the explanation for why this is needed (i.e what you just sent).

@timabbott
Copy link
Sponsor Member

I posted one comment; also, the dragging to reorder fields in the field-edit UI doesn't work for me (it only works in the "new field" UI).

@timabbott
Copy link
Sponsor Member

timabbott commented Aug 3, 2018

OK, I cleaned up that prepartory refactor commit as the 3 commits ending in aa14d24 and merged them, thanks for finding those issues @YJDave! And take a look to see what sort of commit style is best for refactoring; I know it feels laborious, but it's really easy to introduce random bugs in other parts of the UI by accident without either the developer or reviewer noticing without these practices.

To save you a bit of potential rebase work, I pushed to this PR the remaining 2 commits; I'm going to wait on further review on them for you to fix the custom profile fields edit UI I mentioned in my last comment. Thanks @YJDave!

Currently, admin user has to add order of custom-field-choice in
input box to create and edit choice-type custom field.
Remove this input boxes and add drag-drop list of custom-field-choices
using Sortable.js.

Fixes zulip#10129
@YJDave YJDave force-pushed the cp_10129 branch 2 times, most recently from cfc4188 to 990dd5d Compare August 6, 2018 17:17
Admin user must enter at least one choice for choice type fields
in create new custom field form. Admin can not delete all choice
options in form.
Reset delete-btn of choice inputs on choice reordering so that
admin can delete all choice except first choice input option.
@YJDave
Copy link
Collaborator Author

YJDave commented Aug 6, 2018

@timabbott Thanks for reviewing and refactoring!
Updated PR, edit UI is working now.
edit_choice

Before, our edit-field-choice form in custom profile field settings
in admin UI, is rendered when settings modal is loaded not when admin
user clicks on edit-btn.
Admin user open edit-field-form of choice-type-field, do some changes
in choices, discard those changes and close edit-field-form.
When admin user again open this edit-field-form, those discarded
changes are displayed, instead of original choices data.

Fix this issue by re-rendering field choices when admin user clicks
on edit-field-btn.
reset_choices

Currently, our edit-field-choice form in custom profile field settings
in admin UI, is rendered when settings modal is loaded not when admin
user clicks on edit-btn.
Admin user open edit-field-form of choice-type-field, do some changes
in choices, discard those changes and close edit-field-form.
When admin user again open this edit-field-form, those discarded
changes are displayed, instead of original choices data.

Fix this issue by re-rendering field choices when admin user clicks
on edit-field-btn.
@zulipbot zulipbot added size: XL and removed size: L labels Aug 6, 2018
@zulipbot
Copy link
Member

zulipbot commented Aug 8, 2018

Heads up @YJDave, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Sponsor Member

Nice, this looks great, merged after adding a choice_list.off() statement to avoid double-registering click handlers for the + buttons, thanks @YJDave! There's a few small follow-ups to fix:

  • The + buttons only add slots at the end. We should either change that, or perhaps we should just rearrange things so that you can only add a new slot at the end (i.e. just have one +, or even just have a field with a blank value at the bottom that if you start typing into generates a new field below it). @rishig thoughts?
  • I think we could reasonably have the trash-can button available on all of the choices (so you don't need to move around the first one to get rid of it), and just not let you save a field with 0 choices.
  • This probably should just be moved to a new issue since it's a backendy thing and not directly related, but I'm not sure the right thing happens for users who set e.g. their editor to "Emacs" before that choice value is deleted; I think everyone should be reverted to the "Not selected" value, but I'm not sure that's what happens. @YJDave can you verify the bug and if present, open an issue?

@timabbott timabbott closed this Aug 8, 2018
@rishig
Copy link
Member

rishig commented Aug 8, 2018

just have a field with a blank value at the bottom that if you start typing into generates a new field below it

This sounds good to me.

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.

None yet

4 participants