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

hover effect on setting icon and some padding #3352

Merged
merged 3 commits into from
Apr 20, 2021
Merged

hover effect on setting icon and some padding #3352

merged 3 commits into from
Apr 20, 2021

Conversation

anshgo01yal
Copy link
Contributor

Description

Screenshots (optional)

o

Type of change

  • 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
    not work as expected)
  • This change requires a documentation update

Checklist:

  • I have signed the Google CLA https://cla.developers.google.com
  • My code follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have verified my change on multiple browsers on different platforms
  • I have run ./build/all.py and the build passes
  • I have run ./build/test.py and all tests pass

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Thanks!

demo/demo.less Outdated
@@ -32,6 +32,10 @@
.mdl-button .material-icons {
line-height: 36px;
}
//icon is not at the center. this fixes it.
.mdl-button .material-icons-round {
padding-top: 3px;
Copy link
Member

Choose a reason for hiding this comment

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

Is this value dependent on the font size or line height? It would be best to find a robust fix that doesn't have a value dependent on other settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it depends upon line height. Actually hover effect is applied on .mdl-button and there is not enough space from top for .materials-icons-round (x). hence I found padding to solve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. The content of the button is not vertically aligned to the middle, and adding padding to the top is a hack. If you reduce the font size, this isn't needed at all, so I don't think it's the right fix.

Let's experiment a bit and see if we can determine a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

I think I found a better solution.

The button is a display: block at 32x32 and the icon is a display: inline-block at 24x24. Buttons should align their contents to the center by default, but something in MDL may be preventing this from working correctly.

Switching to flex layout makes this easy to fix, though:

.mdl-button--icon {
  /* By default, MDL icon buttons may end up vertically off-center.  This aligns
   * them to the center both vertically and horizontally. */
  display: flex;
  align-items: center;
  justify-content: center;
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yaah! it actually works fine!
gosh! why didn't I think about it!!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

initially there is no flex in .mdl-button--fab. when i put inline-flex in it. the result is even worst
nnnn

i think there is problem in material-icons-round to fix both these issue either we need to put padding top by 3px or introduce line height.
nn2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please let me know if we both have same opinion about it!

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have been more clear. You need both selectors (for --fab and --icon), and you need to set all of the following properties:

.mdl-button--fab,
.mdl-button--icon {
  /* By default, MDL icon buttons may end up vertically off-center.  This aligns
   * them to the center both vertically and horizontally. */
  display: inline-flex;
  align-items: center;
  justify-content: center;
}

Setting display to flex doesn't align things to the center by default. You also need the align-items and justify-content properties, each of which aligns in a different axis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! I got it. sorry I didn't get it for the first time.
I changed the code and everything works fine.
thanks for help.

Copy link
Member

Choose a reason for hiding this comment

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

No worries! Thank you for collaborating with me!

demo/demo.less Outdated
@@ -383,6 +387,10 @@ html, body {
padding: 0 16px;
}

.app-header .mdl-layout__drawer-button .material-icons:hover {
transform: rotate(20deg);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please give a before/after screenshot of this? I am not sure what this does, and your PR description didn't help me figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes rotates the icon when hover
before
o1
after
n1
actually its difficult to find changes through images as its motion effect.

its like settings icon on YouTube player when clicked
y1

Copy link
Member

Choose a reason for hiding this comment

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

I played around with this change, and I found that there are two things I dislike about it:

  1. 20 degrees looks weird. It's a six-sided shape, so intervals of 30 degrees look better than 20 degrees. Otherwise, it just looks crooked.
  2. Having no transition animation makes the change snap to the rotation instantly, which doesn't provide such a pleasing effect.

Having played around with the settings, I think this looks really nice:

.app-header .mdl-layout__drawer-button .material-icons {
  &:hover {
    /* Spin the gear icon in the settings menu when hovered. */
    transform: rotate(90deg);
  }

  /* Animate the spin. */
  transition: transform 0.5s ease-in-out;
}

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. Thanks!

demo/demo.less Outdated
@@ -33,6 +33,14 @@
line-height: 36px;
}

.mdl-button--icon {
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates what you have below, where you do the same for both selectors:

.mdl-button--fab,
.mdl-button--icon {

You can delete this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@joeyparrish
Copy link
Member

Thanks for your contribution! Your PR is in queue for testing by our build bot.

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit ef0c651 into shaka-project:master Apr 20, 2021
joeyparrish pushed a commit that referenced this pull request Apr 22, 2021
Co-authored-by: Anshgoyal <anshgo01yal@users.noreply.github.com>
joeyparrish pushed a commit that referenced this pull request Apr 25, 2021
Co-authored-by: Anshgoyal <anshgo01yal@users.noreply.github.com>
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants