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

Update form input css #8554

Merged
merged 46 commits into from
Mar 27, 2023
Merged

Update form input css #8554

merged 46 commits into from
Mar 27, 2023

Conversation

hurradieweltgehtunter
Copy link
Contributor

Description

Updates layout for form inputs, labels & textareas

Motivation and Context

Text inputs looked condensed and not unified styled.

How Has This Been Tested?

  • Firefox
  • Chrome
  • Safari
  • IE

Screenshots (if appropriate):

Form layout

Before:
grafik

After:
grafik

Input focus highlight

The highlight border uses the gradient of web primary button gradient background.
Before:
grafik

After:
grafik

Comboboxes/Select uses same layout like input

Before:
grafik

After:
grafik

Disabled comboboxes look more disabled & cursor is disabled

Before:
grafik

After:
grafik

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@hurradieweltgehtunter hurradieweltgehtunter added the Status:Needs-Review Needs review from a maintainer label Mar 7, 2023
@hurradieweltgehtunter hurradieweltgehtunter self-assigned this Mar 7, 2023
@ChrisEdS
Copy link

ChrisEdS commented Mar 8, 2023

I love it! It's the details that make the difference! I celebrate every one of your pull requests hard!

@lookacat
Copy link
Contributor

lookacat commented Mar 9, 2023

Very good improvement, looks much better 👍🏼

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Awesome design, as always! 🙂

Oppose to your screenshots, all input fields have a grey (=disabled) background color for me (besides those in modals and drops). Does it happen for you as well?

@hurradieweltgehtunter
Copy link
Contributor Author

Oppose to your screenshots, all input fields have a grey (=disabled) background color for me (besides those in modals and drops). Does it happen for you as well?

Yep, you were right. fixed.

@hurradieweltgehtunter
Copy link
Contributor Author

Added new component OcAlert with different variants:

  • info
  • success
  • warning
  • danger
  • closeable: Renders a close button
  • hasIcon: Renders an Icon
  • customIcon: Renders a custom Icon; enter icon name

grafik

@kulmann
Copy link
Member

kulmann commented Mar 13, 2023

Added new component OcAlert with different variants:

  • info
  • success
  • warning
  • danger
  • closeable: Renders a close button
  • hasIcon: Renders an Icon
  • customIcon: Renders a custom Icon; enter icon name
grafik

While I like the visuals of the OcAlert component I don't agree with the usage in the particular case. The OcAlert seems to be very much standalone, rather disconnected from the OcSelect for the quota. At a glance I don't get the connection of the OcSelect and the OcAlert. I'd prefer to use the line below the (disabled) OcSelect to show the very same message you now show in the OcAlert. E.g. OcTextInput follows the same approach where the line below can be used for a description/explanation but is then reused for an error message if there is e.g. an input validation error.

Screenshot 2023-03-13 at 23 50 38

@hurradieweltgehtunter
Copy link
Contributor Author

While I like the visuals of the OcAlert component I don't agree with the usage in the particular case. The OcAlert seems to be very much standalone, rather disconnected from the OcSelect for the quota. At a glance I don't get the connection of the OcSelect and the OcAlert. I'd prefer to use the line below the (disabled) OcSelect to show the very same message you now show in the OcAlert. E.g. OcTextInput follows the same approach where the line below can be used for a description/explanation but is then reused for an error message if there is e.g. an input validation error.

Ok, I can understand and I would agree. Some explanation why I decided to use an alert:

grafik

Using it like you proposed, my guess is that the message is not visible enough and the user might oversee this.
Also, I would understand it that the subtitle of an input field is for feedback of an input validation - so, depending on the content of that field, e.g. in case of an incorrect input ("Please enter a valid email address"). But in this case it is not dependent on the input, but a general hint. Using the subtitle for this hint would contradict its concept IMO.

But, In the end, it is your decision :)

@hurradieweltgehtunter
Copy link
Contributor Author

Improved alerts in dark mode:

grafik

@ChrisEdS
Copy link

Should that be a info message in any case? I mean there isn't "something wrong", right.

image

@kulmann
Copy link
Member

kulmann commented Mar 14, 2023

Using it like you proposed, my guess is that the message is not visible enough and the user might oversee this. Also, I would understand it that the subtitle of an input field is for feedback of an input validation - so, depending on the content of that field, e.g. in case of an incorrect input ("Please enter a valid email address"). But in this case it is not dependent on the input, but a general hint. Using the subtitle for this hint would contradict its concept IMO.

That's not what I meant. Have a look at the red error message below the text input here: https://owncloud.design/#/oC%20Components/OcTextInput
I'd prefer to be consistent and implement the same behaviour for the OcSelect. Means, removing the Select a quota option or enter your own value and instead show To set an individual quota, the user needs to have logged in once as a warning message. That way it's clear that the message belongs to the OcSelect.

@hurradieweltgehtunter
Copy link
Contributor Author

ok, gotcha. Like this?

grafik

@kulmann
Copy link
Member

kulmann commented Mar 14, 2023

ok, gotcha. Like this?

grafik

Yes 👍

@hurradieweltgehtunter
Copy link
Contributor Author

done from my side, i think. Would need someone to check the added test, which fails, but I don't know why.
@JammingBen could you help me out, again? :)

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Love where this is going! Inputs / form elements feel much better already.

Apart from my comments two things that I'd like to discuss:

  1. we had a lot of (negative!) feedback in the past that the web ui was too big/clumsy in general. With your changes, form fields become bigger - to an extent that forms (e.g. editing a user in the admin settings) use up noticeably more vertical screen space than before. In my opinion a little bit too much. Could you try to reduce that a little bit again?
  2. the blue-ish borders of focussed input fields in dark mode don't match the general appearance of the dark mode in my opinion. I would prefer white. 🤔 Screenshot of the blue-ish border attached.

Screenshot 2023-03-14 at 23 34 17

changelog/unreleased/enhancement-beautify-form-inputs Outdated Show resolved Hide resolved
packages/design-system/src/components/OcAlert/OcAlert.vue Outdated Show resolved Hide resolved
packages/design-system/src/styles/theme/oc-label.scss Outdated Show resolved Hide resolved
@JammingBen
Copy link
Collaborator

done from my side, i think. Would need someone to check the added test, which fails, but I don't know why. @JammingBen could you help me out, again? :)

Sure, I'll try to look into it this afternoon 🙂

@JammingBen
Copy link
Collaborator

I ran the linter and fixed the unit tests. Found some tiny nitpicks on the way:

Text inputs and select inputs now differ in height. Also, select inputs still have that grey background for me 🤔

image

The sharing invite input also has the grey background:

image

@hurradieweltgehtunter
Copy link
Contributor Author

I ran the linter and fixed the unit tests. Found some tiny nitpicks on the way:

Text inputs and select inputs now differ in height. Also, select inputs still have that grey background for me 🤔

image

The sharing invite input also has the grey background:

image

ah damn... yeah. will fix it.
thx

@hurradieweltgehtunter
Copy link
Contributor Author

the blue-ish borders of focussed input fields in dark mode don't match the general appearance of the dark mode in my opinion. I would prefer white. 🤔 Screenshot of the blue-ish border attached.

changed the border-color.
Light mode:
grafik

dark mode:
grafik

@JammingBen
Copy link
Collaborator

One last nitpick, then this is good to go from my side 🚀

You decreased the vertical size of the inputs like Benedikt suggested (which I also prefer), but the select options still have that increased padding you had before:

image

@hurradieweltgehtunter
Copy link
Contributor Author

One last nitpick, then this is good to go from my side 🚀

You decreased the vertical size of the inputs like Benedikt suggested (which I also prefer), but the select options still have that increased padding you had before:

image

Love your attention to details :) will fix

@hurradieweltgehtunter
Copy link
Contributor Author

What is this SonarCloud Code Coverage fail?

@JammingBen
Copy link
Collaborator

It fails because of the condition coverage. But that's okay IMO since this PR (mainly) tackles visual things.

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Nice work! Please remove the duplicated block, then we can merge it :)

packages/design-system/src/tokens/ods/color.yaml Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 27, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

32.3% 32.3% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Really nice job! Found a tiny sizing issue, will post a followup issue since I don't want to delay this PR any further. Thank you!

@kulmann kulmann merged commit f3ca9a8 into master Mar 27, 2023
@delete-merged-branch delete-merged-branch bot deleted the beautify-forms branch March 27, 2023 18:57
@hurradieweltgehtunter
Copy link
Contributor Author

fix here: #8724

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants