looker-open-source / components

Looker's UI Components, Design Infrastructure and more
https://components.looker.com
MIT License
62 stars 31 forks source link

feat(FieldSelect, FieldChips, FieldTime, FieldTimeSelect): Google Material label style #2868

Closed mdodgelooker closed 3 years ago

mdodgelooker commented 3 years ago
github-actions[bot] commented 3 years ago

Coverage report

Total coverage

Status Category Percentage Covered / Total
🟒 Statements 94.84% 7290/7687
🟑 Branches 87.14% 4643/5328
🟒 Functions 93.63% 2101/2244
🟒 Lines 95.38% 7054/7396

Status of coverage: 🟒 - ok, 🟑 - slightly more than threshold, πŸ”΄ - under the threshold

Show files with reduced coverage πŸ”» ### Reduced coverage | Status | Filename | Statements | Branches | Functions | Lines | | :----: | :------------------------------------------ | :------------------ | :------- | :------------------ | :------------ | | πŸ”΄ | components-date/src/FieldTime/FieldTime.tsx | 63.64% (-36.36% πŸ”») | 100% | 33.33% (-66.67% πŸ”») | 70% (-30% πŸ”») | > Status of coverage: 🟒 - ok, 🟑 - slightly more than threshold, πŸ”΄ - under the threshold

Report generated by πŸ§ͺjest coverage report action from 77c2084cef3ad2362b7c3721ee9985c72be0e99c

mdodgelooker commented 3 years ago

@lukelooker I see your point about the placeholder. With text fields, Material seems to allow placeholder but barely mention it in docs: https://material.angular.io/components/form-field/overview#form-field-appearance

But it doesn't seem to exist at all for selects. But the label does move up on focus for selects: https://material-components.github.io/material-web/demos/select/

Do we want to stop just supporting placeholder on Select when externalLabel: false? Or at least I can remove it from our examples...

jhardy commented 3 years ago

My guess is that we have quite a few uses of placeholder's in select, I would be hesitant to remove them.

mdodgelooker commented 3 years ago

@lukelooker I was trippin about externalLabel in the stories – it does show in the controls whether or not you have it in the story args.

As for the Select placeholder... it seems like we need to move the label up immediately on focus to be material-y, but disabling placeholder might be too risky. Maybe we just discourage its use in the docs (once we turn externalLabel off by default in 3.0) or say to only use it with externalLabe={true}? Any other ideas?

ghost commented 3 years ago

@lukelooker I was trippin about externalLabel in the stories – it does show in the controls whether or not you have it in the story args.

πŸ‘

As for the Select placeholder... it seems like we need to move the label up immediately on focus to be material-y, but disabling placeholder might be too risky. Maybe we just discourage its use in the docs (once we turn externalLabel off by default in 3.0) or say to only use it with externalLabe={true}? Any other ideas?

I'm fine with moving ahead as-is – I suspect we'll see people downplay the use of placeholders in designs with this new design and I do think it'll be less disconcerting with real placeholder / label text. :)

ghost commented 3 years ago

Happy to re-review when the stories are cleaned up (use externalLabel = truein template instead of modifying all stories) :)

mdodgelooker commented 3 years ago

@lukelooker I think the externalLabel fixes are all in.