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

Reposition the remove buttons on team management page. #3485

Conversation

arumoy-shome
Copy link

@arumoy-shome arumoy-shome commented Apr 26, 2017

This PR addresses #3477

What does this PR do?

The remove buttons under the Managers section do not line up with the
ones under the Members section. For consistency, this commit applies
similar styles to all remove buttons under the sections mentions above.

How was the issue addressed?

The form that renders the remove button and the manager's gravatar
was wrapped in a col-* class that aligns with the one used under the
members. The css classes on the button were also changed to be similar
to the ones under the members.

Does this cause any side effects?

No however I feel that since we are using the same class on both buttons,
we can perhaps remove the duplicate class (i.e. we can remove either one
of .manager_delete or .member_delete and remove the other to
something more generic such as .person_delete, I am open to
suggestions on the class name)

Further discussions

  1. While I was in there, I noticed that we have inline styles on some elements,
    is this intentional? Can we pull these out into a stylesheet? Perhaps there is a
    chance for some refactor since this is a smell?

  2. We have the same component (i.e. person gravatar + remove button) being
    rendered under both Members and Managers yet they have been done using
    two different approach. Under Managers, we are using a form to issue a post
    request whereas under members we are issuing an AJAX request. Can we
    perhaps pick on method and then extract this component into a partial?

screen shot 2017-04-26 at 4 58 04 am

screen shot 2017-04-26 at 4 58 41 am

please review @tejasbubane @Insti

The remove buttons under the Managers section do not line up with the
ones under the Members section. For consistency, this commit applies
similar styles to all remove buttons under the sections mentions above.
@Insti
Copy link

Insti commented Apr 26, 2017

Thanks @arumoy-shome this looks great.

I'm also really impressed by your pull request message, it is excellent. ⭐

I'll let @tejasbubane comment on the further discussions as he is more familiar with this part of the application than I am. But I think what you have suggested sounds good.

@Insti
Copy link

Insti commented Apr 26, 2017

While you're waiting, you might like to read this: exercism/discussions#113 which may affect how much time and effort you want to put into improving things.

@tejasbubane
Copy link
Member

Thanks a lot for the PR @arumoy-shome ! 🎉

We have the same component (i.e. person gravatar + remove button) being
rendered under both Members and Managers yet they have been done using
two different approach. Under Managers, we are using a form to issue a post
request whereas under members we are issuing an AJAX request. Can we
perhaps pick on method and then extract this component into a partial?

I see duplicate code here not sure why a form was added for managers instead of using ajax.

While I was in there, I noticed that we have inline styles on some elements,
is this intentional? Can we pull these out into a stylesheet? Perhaps there is a
chance for some refactor since this is a smell?

Not sure if it was intentional, but it sounds like a good idea to remove inline styles. But I think it should be a separate PR.

I feel that since we are using the same class on both buttons,
we can perhaps remove the duplicate class (i.e. we can remove either one
of .manager_delete or .member_delete and remove the other to
something more generic such as .person_delete, I am open to
suggestions on the class name)

Since the javascript code uses those classes I suggest not to remove them. To avoid such cases, I would make all javascript code should use classes with js- prefix, in this case js-member-delete and js-manager-delete and css can independently use person-delete.

As @Insti suggested. since we are thinking of reworking on the UI, I am not sure it is worth putting much effort on refactoring the frontend code. As long as we remove the confusion of which delete button was for which user, I think the issue is solved and this PR can be merged.

@arumoy-shome
Copy link
Author

I can address the inline styles issue in a separate PR, meanwhile is this ready to 🚀?

@kytrinyx
Copy link
Member

kytrinyx commented May 6, 2017

This is such a great PR @arumoy-shome! I have to say that starting my morning with this was an absolute delight.

I'm pretty sure that every single nasty thing in there is due to me not knowing anything about front-end development. Every once in a while I get desperate for a feature, and then I just bang rocks together on the front end until I have the button I need (or whatever). Once we get the new site/design implemented, then I'll probably be banned from touching the front end unless I am under strict supervision. We're still doing background research on the UX part, so I anticipate having at least another couple of months of risking me banging rocks together.

@kytrinyx kytrinyx merged commit c7378ff into exercism:master May 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants