Skip to content

Conversation

bugy
Copy link

@bugy bugy commented Oct 22, 2021

As discussed in #52 I tried to remove scss functions from css styles
There are several changes, which I suggest

1. Generic colors

I also tried to reduce the color variants, so I introduced generic variables:

$bg-color
$bg-hover-color-opaque // 4% gray overlay
$bg-hover-color-solid // 4% gray overlay
$bg-focus-color-opaque // 12% gray overlay
$bg-focus-color-solid // 12% gray overlay

$hover-on-secondary-color // e.g. hover on raised button, it should make it lighter
$focus-on-secondary-color // e.g. focus on raised button, it should make it even more lighter
$secondary-color-hover-opaque // 8% overlay, based on secondary color (e.g. time picker hover)
$secondary-color-focus-opaque // 24% overlay, based on secondary color (e.g. time picker focus)

I got these ideas from material design docs: https://material.io/components
Maybe, some names should be adjusted for transparency

2. Better hover/focus states

For the changed components, hover/focus state didn't correspond material guidelines (either they didn't exist at all, like hover in a switch component) or they didn't match the spec (e.g. button hover was lighter, but focus was darker).
So I tried to adjust them to better match specs and also re-use Generic colors

I prepared a test-bed with changed components: https://script-server.net/material-test/index.html (please don't be confused by the server name, I have only 1 running web-server, so just reused it for test-bed)

Would be glad to hear any critique, suggestions. Also, would be nice to agree on the next steps. I guess that refactoring every component in such a manner would be too much work

@bugy bugy closed this Oct 22, 2021
@bugy bugy reopened this Oct 22, 2021
@bugy
Copy link
Author

bugy commented Oct 22, 2021

Hi @DanielRuf @LoganTann, I followed the instructions from Daniel, and when I did force-push of the clean branch, the previous PR got closed. When I pushed new commits, it didn't get restored, so I had to create a separate PR

Also, cherry-picking was somehow weird, it didn't pick all the changes, so in the end, I just copied the whole sass folder from my old branch and committed it with a new message.

Should we try to restore my previous PR somehow? Or I can also move the description to this new PR.

And another problem: are tests supposed to run for so long? For the commit before last, they are running already almost for 2 hours

@DanielRuf
Copy link

and when I did force-push of the clean branch

Probably because the commits were removed from the branch so it was exactly the same which is already in v2-dev. Then GitHub automatically marks this as closed afaik.

@DanielRuf
Copy link

Should we try to restore my previous PR somehow? Or I can also move the description to this new PR.

Let's use this new PR here. The old one is not relevant anymore.

@DanielRuf
Copy link

And another problem: are tests supposed to run for so long? For the commit before last, they are running already almost for 2 hours

In v2-dev we might have to merge the changes from main as this was a problem in the test setup.
See 9d03c4f and the other commits from 18th of September at https://github.com/materializecss/materialize/commits/main

@DanielRuf
Copy link

There is a conflict in sass/components/_sidenav.scss that has to be resolved.

# Conflicts:
#	sass/components/_sidenav.scss
@DanielRuf DanielRuf requested a review from a team October 22, 2021 21:07
@bugy
Copy link
Author

bugy commented Oct 22, 2021

Seems to be working now, thanks!
I'll have to check if anything should be adjusted after merging changes from master. The code/tests work, but may be I would need to introduce more variables

@RamonvdW
Copy link

RamonvdW commented Oct 23, 2021 via email

@bugy
Copy link
Author

bugy commented Oct 25, 2021

Hi @RamonvdW, thanks a lot for the feedback. I checked the issues, you can answer my feedback below:

  • DROP ME! Button stays highlighted after clicking.

It's the same as in default behaviour: https://materializecss.com/dropdown.html (but previously, the focused color for the button was dark, which is wrong according to material UI)

  • Buttons cannot be reached with tab

The same as in default behaviour: https://materializecss.com/buttons.html. Only <button> elements are focusable by default

  • DROP ME! Button requires Enter instead of Spacebar

The same as in default behaviour :( https://materializecss.com/dropdown.html

  • Tab beyond DROP ME! Seems to enter the drop-down menu that is not visible.

True, thanks. That's the issue with my testbed, I'll redeploy a newer version

  • SideNav can be opened with Enter but there is no way out (Escape does not work)

The same as in default: https://materializecss.com/sidenav.html

  • After opening SideNav, tab continues on the main window, not in the sidenav

True, it's the issue with my testbed. I'll redeploy fixed version

  • Tabs must be selected with Enter instead of Spacebar
  • Time picker must be opened with Enter instead of Spacebar
  • Time picker cannot be operated with the keyboard. Escape does work though.
  • Switches can be operated with the Spacebar, but not with Enter.

The same as in default behaviuour: https://materializecss.com/tabs.html https://materializecss.com/pickers.html https://materializecss.com/switches.html

So, I would suggest to open separate tickets for this issue. My Pull Request only adjusts colors of the elements, not their behaviour.

@bugy
Copy link
Author

bugy commented Oct 25, 2021

Hi @DanielRuf @LoganTann I'm done with my changes. Should we merge them to v2?

display: inline-block;
font-weight: 300;
color: #757575;
font-weight: 400;

Choose a reason for hiding this comment

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

Just out of curiosity, why changing the font weight ?

Copy link
Author

Choose a reason for hiding this comment

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

I just aligned font-weight style, as how it is actually looking on materializecss demo page:
(right panel here: https://materializecss.github.io/materialize/scrollspy.html)

image

Choose a reason for hiding this comment

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

Fine, thanks for the answer !

Copy link

@LoganTann LoganTann left a comment

Choose a reason for hiding this comment

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

Everything seems fine, just awaiting the answer for my comment. (edit: done)
Thanks for taking your time solving that long issue, the code looks better now !
Note : I only have the member role, so I don't have the rights to merge.

cc. @materializecss/members-write-access

@DanielRuf DanielRuf merged commit 84a4547 into materializecss:v2-dev Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants