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

Added ability to change tooltip animate opacity. #23

Merged
merged 5 commits into from
Sep 19, 2020
Merged

Added ability to change tooltip animate opacity. #23

merged 5 commits into from
Sep 19, 2020

Conversation

june07
Copy link

@june07 june07 commented Sep 13, 2020

Proposed changes

Currently using this library for NiM and would like to keep transparent tooltips as seen here:
NiM Transparent Tooltips

This small change adds the ability to change tooltip default opacity using options. The ultimate result are tooltips that can be transparent.

M.Tooltip.init(tooltipElement, {
     _animateInOpacity: 0.7
})

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@june07
Copy link
Author

june07 commented Sep 13, 2020

FYI, that update to the _footer.html file was due to the fact that I initially forked Dogfalo/materialize first... that's where the change came from. To eliminate the file from this PR I just removed the change.

@Smankusors
Copy link
Member

hmm IMO, I don't see why there's any use case to change the animate opacity? It's kinda hard to read if the opacity below 0.5...

also why the added option begin with underscore?

@june07
Copy link
Author

june07 commented Sep 13, 2020

And the opacity above 0.5? As you can see in the gif, opacity is set at 0.7. Without this change the tooltip simply gets stuck as overriding the opacity in css breaks the element hiding.

The underscore was based on the function name the setting is found in tooltip.js.

_animateIn() {
      this._positionTooltip();
      this.tooltipEl.style.visibility = 'visible';
      anim.remove(this.tooltipEl);
      anim({
        targets: this.tooltipEl,
        opacity: 1,
        translateX: this.xMovement,
        translateY: this.yMovement,
        duration: this.options.inDuration,
        easing: 'easeOutCubic'
      });
    }

@Smankusors
Copy link
Member

And the opacity above 0.5? As you can see in the gif, opacity is set at 0.7. Without this change the tooltip simply gets stuck as overriding the opacity in css breaks the element hiding.

even though 0.7, for me it's hard to read 😵

The underscore was based on the function name the setting is found in tooltip.js.

I mean... you can see duration: this.options.inDuration, it's not begin with underline. Also the documentation should be updated so the other users know about this option 😉

@Smankusors Smankusors requested a review from a team September 13, 2020 19:17
Copy link

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

I think we can remove the underscore.

More options are always a good idea.
But keep in mind that by default materialize strictly follows Material Design spec by Google.

@DanielRuf
Copy link

Also the documentation should be updated so the other users know about this option 😉

Definitely.

@june07
Copy link
Author

june07 commented Sep 13, 2020

Renamed the option per feedback. Also updated doc.

@DanielRuf DanielRuf requested a review from a team September 13, 2020 20:32
@Pierstoval
Copy link

Shouldn't this be handled by custom CSS rules instead of a feature right in Materialize? 🤔

@DanielRuf
Copy link

And the opacity above 0.5? As you can see in the gif, opacity is set at 0.7. Without this change the tooltip simply gets stuck as overriding the opacity in css breaks the element hiding.

@Smankusors Smankusors merged commit 8fce193 into materializecss:v1-dev Sep 19, 2020
@DanielRuf DanielRuf added component: tooltip enhancement New feature or request labels Sep 20, 2020
JayDijkstra pushed a commit to JayDijkstra/materialize that referenced this pull request Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tooltip enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants