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

WIP: refactor(all): improve theming and CSS consistency #232

Open
wants to merge 91 commits into
base: master
from

Conversation

@ben-girardet
Copy link
Contributor

ben-girardet commented Nov 26, 2019

Please find here the WIP, targeting the ux-button component at the moment to improve theming and CSS consistency.

If you want to see it in practice, you can run the local app (detailed instruction in the main README)

cd app
npm ci
npm run dev

If you want to have quick look, here is a published dev version: https://ux.mintello.com
It should look like the GIF below and allow you to play with Aurelia UX core variables and ux-button theme properties and see the result live.

ezgif-1-88e670a2f35b

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 3, 2019

@ben-girardet Just wanted to say thanks for putting this together and that I haven't forgotten about it. I'm planning to review it this week.

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Dec 3, 2019

@ben-girardet it'd be so nice, if we are able to use the test app, as some sort of e2e for the CI for ux. Would you be interested in writing some tests for this? So we have some thing that can guarantee things still work properly on the surface. Though I'm not sure which style of test we should be doing, e2e or just some integration

@ben-girardet

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 3, 2019

@bigopon I agree that would be very useful to have tests for Aurelia UX. However I don't have any experience whatsoever in E2E testing. If someone could put together a few tests as exemples I'm sure I could contribute and add more.

At the moment I use the test app to manually test things out. As I'm using the library in several projects I already have some ideas of issues that need to be addressed (hence this topic and PR).

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Dec 3, 2019

@ben-girardet yes thats the plan, once this in, I will start a PR to set up test infra for the test app, then write some sample tests and we can start ramping up in this area. Should help greatly. For now, i think we have the luxury of doing manual testing

# Conflicts:
#	app/tsconfig.json
@ben-girardet

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 4, 2019

@bigopon I've merged your latest build-fix into this branch and PR.

It builds correctly for the components, but when I try to run the dev app, I get an error with npm ci

  • I've cleaned node_modules and app/node_modules before to try
  • I've followed the npm ci, npm run bootstrap, npm run build steps on project root
  • And then: cd app and npm ci which result in:
npm ci
npm ERR! aurelia-binding not accessible from @aurelia-ux/core

Any idea how to fix this ?

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Dec 4, 2019

@ben-girardet can you try adding aurelia-framework to here

"aurelia-logging-console": "^1.1.1",

Basically npm i aurelia-framework inside app folder`

@ben-girardet

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 4, 2019

That was it! Thanks @bigopon

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 5, 2019

@ben-girardet This is looking good. I'm quite happy with the approach. @serifine Do you have time to make a review? Any reason not to continue in this direction?

@serifine

This comment has been minimized.

Copy link
Member

serifine commented Dec 7, 2019

Sorry for the delay, it's been a very busy week for me. I just took a look at the changes so far and everything looks great to me! I think we can keep going this direction no problem.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 7, 2019

Thank you @serifine. No need to apologize.

@ben-girardet Thanks again for this contribution. Just ping us when you are ready for final review before merge and we'll jump back in and move this forward.

@ben-girardet

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Dec 9, 2019

@EisenbergEffect @serifine @bigopon

I ping you all to keep you up to date with how I move forward with this PR. I have to take some decisions along the way and I try to summarize theme here. Feel free to comment and help me navigate this issue with the best respect of web standards and Aurelia values.

Core

  • Add an alphaGrey swatch (60a6986)
  • Add a controlLabel style property (576d051). This new property is used to set the default label color for controls and is also used in default border colors (depending on state)

Input (f64f189)

  • Simplify the theme properties, especially:
  • Add an activeColor theme property (defaults to primary) to set the color of the input when active. This will be used for floating label, border, etc... I find that most of the time (if not all the time) there is only one active color needed and it would be painful to set labelActiveColor, borderBottomActive color and so on...
  • Add the option to have both filled and outline input (following material design guidelines)
  • For filled input, I do display a light bottom border by default (material guideline). This border will grow with scaling effect when input becomes active
  • When the input is not active, I default the bottom border to the controlLabel property
  • For outline input, the hover effect do not impact background but the border (so that the floating label can correctly hide the border at the top when going over the border). Therefore I default this border hover color to the controlForeground property

In essence, I try to follow material design as closely as possible and group styling properties when it makes the most sense. Let me know if you agree with the above decisions, need more informations or prefer to change things.

Demo Update
AureliaInput

Next

  • So far I have added 3 new core properties: disabledBackground, disabledForeground and controlLabel. I think that they can really help designers to fine tune their global themes without needing to set specific values for each components. I would like to go further by adding an error property (most controls need an error color).
  • When ux-input looks good I plan to continue with the other control components
@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Dec 10, 2019

@ben-girardet This all looks great to me. Please continue 😄

- `error`
- `errorContrast

The `error` property is used as main Error color. The `errorContrast` is available when when the `error` color is used as background and we want to write text on this surface.
Because it do not make sense according to material design.
@ben-girardet

This comment has been minimized.

Copy link
Contributor Author

ben-girardet commented Jan 23, 2020

@EisenbergEffect @bigopon @serifine

FYI: ux-datepicker, ux-card and ux-list are now ready for review.

I'm planning to work on ux-checkbox, ux-radio and ux-switch tomorrow.

Question: is the HTML markup for checkboxes and radio OK in the showcase file ?

I ask because I want to make sure I follow the original plan regarding markup. My question includes the wrapping around with ux-field

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Jan 23, 2020

@ben-girardet 👌 nice, amazing work. Can't wait to see the updated demo
I will review soon

if (focused === true) {
this.element.classList.add('ux-datepicker--focused');
} else {
this.element.classList.remove('ux-datepicker--focused');
}
Comment on lines 204 to 208

This comment has been minimized.

Copy link
@bigopon

bigopon Jan 24, 2020

Member
this.element.classList.toggle('ux-datepicker--focused', focused === true)
readonly.bind="readonly & booleanAttr"
aria-disabled.bind="disabled & booleanAttr"
aria-readonly.bind="readonly & booleanAttr"
click.trigger="focusInput()">

This comment has been minimized.

Copy link
@bigopon

bigopon Jan 24, 2020

Member

does this mean that any click, either on the input, the datepicker container, or the picker dialog will focus the textbox?

This comment has been minimized.

Copy link
@ben-girardet

ben-girardet Jan 24, 2020

Author Contributor

No it will not open the picker dialog, only focus the element. The the idea here is to mimic the behavior of ux-input where any part of the element is clickable to give focus.

@@ -50,6 +59,10 @@ export class UxDatepicker implements UxComponent {
public bind() {
this.processAttribute('placeholder');

if (this.autofocus || this.autofocus === '') {
this.focused = true;

This comment has been minimized.

Copy link
@bigopon

bigopon Jan 24, 2020

Member

when setting this and the element is not yet attached, will it actually be focused as expected by autofocus?

This comment has been minimized.

Copy link
@ben-girardet

ben-girardet Jan 24, 2020

Author Contributor

Yes it works. At least from my test in browser (chrome). Again, I have mimic the behavior of ux-input.

@@ -4,49 +4,213 @@
box-sizing: border-box;
flex-wrap: wrap;
flex-direction: row;
align-items: center;
align-items: flex-start;

This comment has been minimized.

Copy link
@bigopon

bigopon Jan 24, 2020

Member

what visual bug/styling annoyance that you encounter with this?

This comment has been minimized.

Copy link
@ben-girardet

ben-girardet Jan 24, 2020

Author Contributor

On top of my head I remember that I first had issues positioning the label, especially when the textarea grow to fit its content. If I remember correctly, aligning items with flex-start helped me to achieve right positioning with floating labels and outline labels.

@bigopon

This comment has been minimized.

Copy link
Member

bigopon commented Jan 24, 2020

@ben-girardet looking good to me. thanks for the work. Beside a few minor comments, I think we are gonna have a good update for this. Can't wait to see the next version of the demo. I'll try to setup testing infra soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.