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

Fix action classes on admin tables #2336

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Oct 30, 2017

Hovering on any tables actions link/button, for example into:

/admin/products/:slug/stock

will trigger a JS code that adds a class to the closest row (the row where that action link is):

before-click

But when clicking on that link, a new template is rendered to show a new form that reflects the action needed. When this happens no mouseleave event is called on the link/button so it cannot restore the right state of the row.

edit

In this screenshot we can see there are 2 events classes (action-edit e action-cancel) but only the action-cancel class should be present.

The real UX issue happens clicking on the cancel button. The action-cancel class will remain on that div, even if we have done editing, this will change the stock form state color hovering for edit again:

after-click

This PR fixes this behavior by programmatically triggering mouseleave event each time we render the product stock template.

I feel like we have the same problem elsewhere but, before investigate further, I'd like to have some feedback on this solution since it seems a bit hacky to me. Unfortunately I cannot find a better one at the moment.

EDIT

At the end I've used MutationObserver to understand when inner elements of the tr change (so when it is replaced with content of the new template rendered).

MutationObserver is pretty new. It's the first time I use it but it seems like it's fully supported by browsers we should care for admin.

@kennyadsl kennyadsl self-assigned this Oct 30, 2017
@kennyadsl kennyadsl added changelog:solidus_backend Changes to the solidus_backend gem UI labels Oct 30, 2017
@jhawthorn
Copy link
Contributor

jhawthorn commented Oct 30, 2017

We had a similar problem with tooltips. I solved it by polling to check that the tooltip still exists. It's pretty hacky but we might consider implementing something similar for this.

https://github.com/solidusio/solidus/blob/master/backend/app/assets/javascripts/spree/backend/components/tooltips.js#L6-L24

The comment saying "This may be unnecessary in a future version of bootstrap" is no longer true, and they now consider the problem a WONTFIX (which is fair, given how hacky our solution is) 😞

@kennyadsl
Copy link
Member Author

kennyadsl commented Nov 1, 2017

@jhawthorn Thanks for your feedback. Another approach could be using MutationObserver to understand when childList of that tr changes. When this happens we could remove action- classes from that element:

  $('table').on("mouseenter", 'td.actions a, td.actions button', function(){
    var tr = $(this).closest('tr');
    var klass = 'highlight action-' + $(this).data('action')
    tr.addClass(klass)
    tr.prev().addClass('before-' + klass);

    removalObserver = new MutationObserver(function(mutations) {
      // disconnect itself when content is changed, a new observer will
      // be attached to this tr by the new mouseenter event on the new
      // element rendered by the template.
      this.disconnect();

      // remove '.highlight' and '.action'- classes related to this mouseenter event
      tr.removeClass(klass);
    });
    removalObserver.observe(tr.get(0), { childList: true, attributes: false });
  });

I like this approach because code related to this behavior is contained into code that defines it, so we don't have to add code into other js files (like I did in current commit). Also this should fix this issue everywhere it is present.

The only thing that I'm not able to do at the moment is disconnecting the MutationObserer attached to the tr on mouseleave. If you like this approach I can try to find a solution.

Clicking on an action button in a table, a new template for the row
is rendered under current mouse coordinates. This was not making fire
the mouseleave callback for that element.

This commit introduces a MutationObserver that takes the same mouseleave
actions when elements inside that row changes. Also, move the mouseleave
event listener into the mouseenter callback. This way we have the
instance of the MutationObserver and we can disconnect it properly to
avoid aving several mutationobserver attached to the same elements due
to several consecutive mouseenter/mouseleave on that row.
@kennyadsl kennyadsl force-pushed the improve-product-stock-form branch from 3c0b94f to 94781aa Compare November 18, 2017 13:46
@kennyadsl kennyadsl changed the title Fix action classes of product stock form Fix action classes on admin tables Nov 18, 2017
@kennyadsl
Copy link
Member Author

kennyadsl commented Nov 18, 2017

I've updated the PR using MutationObserver, much better now in my opinion!

@jhawthorn thanks for the suggestion of moving the mouseleave event binding into the mouseenter callback, so we have the instance of the MutationObserver available.

@jhawthorn jhawthorn merged commit ba0de82 into solidusio:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants