onehilltech / ember-cli-mdc

ember-cli addon for material-components-web
Apache License 2.0
44 stars 15 forks source link

MDC slider depends heavily on initially rendered position #7

Closed jamesstaub closed 3 years ago

jamesstaub commented 5 years ago

I'm wondering if it's in the scope of this library to implement a fix for a known issue with the MDC slider.

https://github.com/material-components/material-components-web/issues/4365

In my use case, the slider is rendered in an off screen component, which then slides into view. The mouse dragging range of the slider is based on the initial rendering position, so it becomes impossible to use if the original position was offscreen.

I implemented the following hack to re-render the component when the user clicks the slider, but I'm wondering if you have any ideas for a more robust solution.

export default MDCSlider.extend({
  layout,

  getPosition() {
    return this.element.getBoundingClientRect();
  },

  reBind() {
    const { x, y } = this.getPosition();
    if (x !== this.x || y !== this.y) {
      this.willDestroyElement();
      this.didInsertElement();
    }
  },

  didInsertElement() {
    this._super(...arguments);
    // cache the x,y position to determine if re-render is necessary
    const { x, y } = this.getPosition();
    this.setProperties({ x, y });
  },

  didUpdateAttrs() {
    this.reBind();
  }
});

To reproduce the behavior I created this example

// application.hbs
<style>
  .menu {
    border: solid 1px;
    transform: translateX(-100%);
    transition: all .5s;
  }
  .open {
    transform: translateX(0);
  }
</style>

<div class="menu">
  {{mdc-slider 
    min=0
    max=100
    value=50
  }}
</div>

<button {{action "open"}}>open</button>
// controllers/application.js

export default Controller.extend({
  actions: {
    open() {
      document.querySelector('.menu').classList.add('open');
    },
  }
});

thanks!

hilljh82 commented 5 years ago

@jamesstaub Yes, it is in the scope of the library to handle this problems. I had to provide a similar fix for handling error states with text field and text area components. We just have to be mindful that if this is fixed in the material-components-web, then we need to use that solution in favor of this one.

I think there could be a better solution that does not require the necessary checks if the initial your use case never comes up. I'm think of something along the lines of the State Pattern could be a solution approach since it would remove the Code Smell of using if-else instead of inheritance and polymorphism.

hilljh82 commented 5 years ago

@jamesstaub Another solution, that does not require modifying the initial slider, is to subclass the mdc-slider to create a specialized one for being initially offscreen. I think the former solution would be a better solution than this one since the specialized slider requires you to know if your slider is initially offscreen.

hilljh82 commented 5 years ago

@jamesstaub I wanted to give you an update on this issue. I know how to fix the problem. I am trying to create a clean example that illustrates how to use the fix. As a preview, the mdc-slider component will have a forceLayout attribute that will map to the layout() method on the material-components-web slider. This seems to solve the problem you are experiencing. You, however, will have to do something extra work at the application-level to trigger this update. The example will illustrate how this can be done.

hilljh82 commented 5 years ago

@jamesstaub I have merged a fix into master, and will be making a bug fix release. The example illustrating the problem and solution can be found in tests/dummy in mdc-slider package. All the entities that have a issue7 prefix are part of solution example.

To better explain the solution, you must signal the requestLayout attribute on the mdc-slider. In order for this to happen, the slider must be a child of an Ember component. In your example above, you have a raw div element. This needs to be converted to an Ember component so we can bind to in the Handlebars template. Here is the updated handlebars code:

{{#issue7-menu open=open as |menu|}}
  {{mdc-slider min=0 max=100 value=50 requestLayout=menu.isOpenComplete}}
{{/issue7-menu}}

{{#unless open}}
  <button {{action (mut open) true}}>open</button>
{{else}}
  <button {{action (mut open) false}}>close</button>
{{/unless}}

In this example, we are going to bind the issue7-menu component to the menu variable. This allows us to use it within the scope of the associated issue7-menu component. Next, we are going to update the issue7-menu component template code to yield itself.

{{yield this}}

Lastly, we are going to implement the issue7-menu component to use classNameBindings and listen for the transitionend event.

import Component from '@ember/component';
import layout from '../templates/components/issue7-menu';

export default Component.extend({
  layout,

  classNames: ['issue7-menu'],
  classNameBindings: ['open:issue7-open'],

  // Tracks the open state of the menu.
  open: false,

  // Let us know when the open is complete.
  isOpenComplete: false,

  _transitionEndListener: null,

  init () {
    this._super (...arguments);

    this._transitionEndListener = this._transitionEnd.bind (this);
  },

  didInsertElement () {
    this._super (...arguments);

    // Listen for the transition end event.
    this.element.addEventListener ('transitionend', this._transitionEndListener);
  },

  willDestroyElement () {
    this._super (...arguments);

    this.element.removeEventListener ('transitionend', this._transitionEndListener);
  }

  _transitionEnd () {
    // Update the open complete state.
    const isOpenComplete = this.element.classList.contains ('issue7-open');
    this.set ('isOpenComplete', isOpenComplete);
  }
});

For completeness, here is the SASS.

.issue7 {
  &-menu {
    border: solid 1px;
    transform: translateX(-100%);
    transition: all .5s;
  }

  &-open {
    transform: translateX(0);
  }
}

To recap, the code above is allowing the menu component to listen for the transitionend event from when the issue7-open style is added to the component. When this event fires, we update the isOpenComplete state. In the template code, we are binding the issue7-menu component to the menu variable. We then bind the menu.isOpenComplete to the requestLayout variable. Now, when we transition the menu to open, the slider will request it's layout to be updated. When we close the menu, it will reset the state of isOpenComplete so it will fire the next time we open it.

From what I am learning about this issue, this is really the only way you can resolve this issue. Implementing the solution within the material-components-web slider component will actually be very inefficient because resolving the layout issue requires application-level knowledge.

hilljh82 commented 5 years ago

v 0.74.16 has the fix for this issue.

hilljh82 commented 5 years ago

@jamesstaub I am going to go ahead and close this issue. If the solution does not resolve your problem, please reopen this issue.

jamesstaub commented 5 years ago

Sounds good, thank you.

hilljh82 commented 5 years ago

@jamesstaub I am going to reopen this issue to ask a question.

What MDC component were you trying to use the slider in that was causing this issue? Or, were you trying to use the slider in a custom component you were developing that had animation?

jamesstaub commented 5 years ago

I'm using it inside an ember-modal-dialog which isn't animated per se but uses ember-wormhole, so it gets moved around the DOM after render.

hilljh82 commented 5 years ago

@jamesstaub The reason I ask is because I created a polyfill package for the side sheet component while the real one from material-components-web is being developed. The side sheet has a slider, and was running into the same problem you experienced. I was planning to update the side sheet to have built in events for when different animations end. I was also thinking of pushing this to the animation package via a mix that all components using animation can utilize. If you were using a mdc component that had animation, then I would ask which one you were using so I have an additional test case.

jamesstaub commented 5 years ago

ok gotcha. If you take such an approach and it solves the problem for {{mdc-dialog}} then I will likely switch to that from ember-modal-dialog.

Thanks for the update

hilljh82 commented 3 years ago

I am closing this issue for now because the new material-components-web may have fixed this issue internally, which ember-cli-mdc 2.x is built on. If not, please re-open it so we can investigate.

jamesstaub commented 3 years ago

@waissbluth