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

fixed #29 #92

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

stephanebastian
Copy link

@stephanebastian stephanebastian commented Mar 10, 2023

This is a WIP. textfield-filled is 90% done. outlined is completely broken

@robinlahtinen : This is the first stab at fixing #29 .
It's currently working on Firefox and Chrome. I believe it should be working with fairly old browsers and the default mode is 'populated' so that old browsers don't supporting fancy css would work.
Note that I changed classnames to be 'textfield-' instwerd of 'fieldtext-' and such.
Whenever you have time I would appreciate any feedback you may have.

@robinlahtinen robinlahtinen self-requested a review March 10, 2023 19:04
Copy link
Member

@robinlahtinen robinlahtinen left a comment

Choose a reason for hiding this comment

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

Currently dist folder should only be updated on new releases. Reason being, that the website uses main as a production branch and automatically updates from it.

Copy link
Member

@robinlahtinen robinlahtinen left a comment

Choose a reason for hiding this comment

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

I'll do a proper full review later, but from a quick glance (both code and running it) the current changes look good and the code is clean!

@stephanebastian
Copy link
Author

@robinlahtinen I believe this PR fixes #29. Could you please do a full review whenever you have some spare time ?
As a side note, you will see that I've added the file _md.scss and updated the import section. When I got started on implementing this feature, I found that it was somewhat a pain to prefix theme related variables with 'light', or state related variables with state.$varName (same goes for typo and such). So I added the file md.scss. The goal is to import only this file (@use 'md' as *) so we reference variables as $md-sys-typescale-body-large-size.
It's not a big deal but I found it easier when doing a cut/paste of variables from the material design spec. Wondering if you´d feel the same. Cheers

@stephanebastian
Copy link
Author

Not sure why the labeler is failing. Might be related to this issue though: actions/labeler#12
it seems to be a permission issue.
Do you have an idea on how to fix it?

@robinlahtinen
Copy link
Member

robinlahtinen commented Mar 15, 2023

I found that it was somewhat a pain to prefix theme related variables with 'light' [...] It's not a big deal but I found it easier when doing a cut/paste of variables from the material design spec.

Good idea! Material Design Light supports dark theme, so that should also be taken in to concern.

Could you please do a full review whenever you have some spare time ?

Code looks good and seems to be up to the specifications mostly. It is missing support for dark theme and some lesser problems I referenced directly on the code reviews below.

  • When hovering mouse over the text field, the cursor doesn't change the type to text (shape of I-beam) mode on the inner sides.
  • Text fields are missing some of the status effects, eg. on-hover color changes.

scss/_fields.scss Outdated Show resolved Hide resolved
site/src/index.njk Outdated Show resolved Hide resolved
@stephanebastian
Copy link
Author

@robinlahtinen I've made the changes you requested, splitted the file in two files, one for filled text fields and another one for outlined text fields. Could you please review one more time and merge if its ok? Thx

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.

Text fields uses :has() CSS pseudo-class that many browsers don't support yet
2 participants