Skip to content
This repository has been archived by the owner on Jun 13, 2022. It is now read-only.

Add ability to style button hover #149

Merged
merged 7 commits into from
May 31, 2018

Conversation

ArbiterOfBuffoonery
Copy link

This change stems from the flex-toolbar issue discussion.

@ArbiterOfBuffoonery
Copy link
Author

The build check warning about adding a function inside a loop has made me realize that if there is more than one property they will not all be applied since the onmouseenter and onmouseleave functions would just point to the last property...I'm going to close this and think about it some more as I need to iterate all the properties in the function instead of how I am doing it now, but I'm not quite sure how that should work at the moment.

@UziTech
Copy link
Collaborator

UziTech commented May 25, 2018

You would also have a problem that button and propName are changed by a for loop so they will always be the last value in the array inside the anonymous functions.

https://stackoverflow.com/a/13977142/806777

I would try creating a function that take the btn and return a function for the listeners.

something like:

if (btn.hover && !disable) {
  button.element.addEventListener('mouseenter', this.onMouseEnter(btn), {passive: true});

...

onMouseEnter(btn) {
  return function () {
    for (const propName in btn.hover) {
      const style = btn.hover[propName];
      this.style[changeCase.camelCase(propName)] = style;
    }
  };
},

Copy link
Collaborator

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

Make sure to use tabs and align the for loop in onMouseEnter

btn['preHoverVal'] = new Object();

for (const propName in btn.hover) {
btn.preHoverVal[propName] = this.style[changeCase.camelCase(propName)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you save the changeCase.camelCase(propName) to a variable instead of calculating it every time it is needed?

Also if you change the propName in btn.preHoverVal[propName] to the camel case variable you won't need to calculate the camel case prop name in onMouseLeave

@ArbiterOfBuffoonery
Copy link
Author

Sorry about the formatting issue, I should have caught that...

Also, I should have caught the camel case conversion. I even thought about this briefly, but instead of reading up on what it was actually doing, I (incorrectly) assumed it was something that was "needed" and dismissed it without further thought. The name even implies it is a string manipulation function that would be more efficient to run once 😞.

@UziTech
Copy link
Collaborator

UziTech commented May 30, 2018

No problem. I will merge this and create a new release tonight. Thanks a lot for this, it is incredibly useful. 💯

@ArbiterOfBuffoonery
Copy link
Author

Happy to help! Thanks for providing guidance and feedback, it was good information to learn.

@UziTech UziTech merged commit 33db620 into cakecatz:master May 31, 2018
@UziTech
Copy link
Collaborator

UziTech commented May 31, 2018

released in v2.1.0 Thanks again

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants