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.

Sign up for GitHub

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

Jump to bottom

MEN-7838 - text input appearance #271

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

MEN-7838 - text input appearance #271

wants to merge 10 commits into from

Conversation

mzedel
Copy link
Contributor

@mzedel mzedel commented Dec 17, 2024

No description provided.

@chiachenglu
Copy link
Contributor

Nice, thank you! @mzedel Is there also a way for me to see the visual changes too? like clicking around to see how it appears in different places in UI, or I can only see after it is merged and see it on staging.mender.io?

aleksandrychev
aleksandrychev previously approved these changes Dec 18, 2024
View reviewed changes
@mzedel
Copy link
Contributor Author

mzedel commented Dec 18, 2024

No unfortunately we don't have preview deployments, but you can run it locally using the image built in CI. I'm sure @alfrunes can help you set this up 😉

@chiachenglu
Copy link
Contributor

Ok, I ran it locally using the image built. I noticed some inconsistencies on the page. For example, some places failed to update to the new variant or did update to the latest variant but did not update its different states. For example, the default state looks correct, but the focus state still looks like the standard variant. Attached are some of the noticeable inconsistencies I noticed. I am guessing these are built in different classes or something which is why it doesn't get to be updated. Nevertheless, let's try to update as much as possible before we publish the changes to make sure we have a consistent look in the most of the pages and features. @mzedel

image (1)
image (3)
image (2)
image (4)
image (5)
image (6)

@chiachenglu
Copy link
Contributor

@michaelntech I am considering to also update the select property from standard to outlined variant to get the consistent input field for Mender GUI, What do you think?
Screenshot 2024-12-18 at 11 18 16

@michaelntech
Copy link
Member

@michaelntech I am considering to also update the select property from standard to outlined variant to get the consistent input field for Mender GUI, What do you think?

Yes, I agree with using outlined everywhere

@chiachenglu
Copy link
Contributor

Awesome, @mzedel kindly update the select property for us as well.

chiachenglu
chiachenglu requested changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@chiachenglu chiachenglu left a comment

Choose a reason for hiding this comment

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

Please adjusted the inconsistent places (see above comments).

@mzedel
Copy link
Contributor Author

mzedel commented Jan 9, 2025 •
edited
Loading

Please adjusted the inconsistent places (see above comments).

@chiachenglu unfortunately I don't think I will get to it before the end of the sprint 😕
I did get to renew the changes to the codebase changes that rolled in in the meantime.
There I stumbled across some more inconsistencies when at least rebasing the existing changes, mainly height differences in the release section - should these be aligned to the taller appearance?
And some of the spots you pointed out (email address, not sure what was the other) are disabled/ not editable/ need to be turned editable first, should they also be shown in the bordered style already when disabled?

@chiachenglu
Copy link
Contributor

That is no problem. I left the comments so we don't forget to update them.
Although I am not sure why there are height differences, I guess they are from different sizes of the text input component perhaps (medium and small)? In general, I would expect the text field component to behave and look consistently in its different states (default, hover, focus, disable, and error), whether it is editable or not. So yes, outlined style in disable state too! 👍🏻

You can view the text field component and how it looks in different states with different properties here: https://www.figma.com/design/e2WPEGusQtx3JnKtOO2san/%F0%9F%8E%B8(UI)-Mender-Design-System-(WIP)?node-id=6570-46740&t=0s73ZMr7eAkjqaen-1

@mzedel mzedel requested a review from chiachenglu January 17, 2025 14:42
@mzedel
feat(gui): aligned text input appearance with MUI updated guidelines
c1a4bf4 
Ticket: MEN-7838
Changelog: Title
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel mzedel force-pushed the men-7838 branch 2 times, most recently from 127c746 to 3e23b14 Compare January 17, 2025 19:53
@chiachenglu
Copy link
Contributor

chiachenglu commented Jan 21, 2025 •
edited
Loading

Thanks for the fixes, overall looks good! Only found following things that needs to be final adjusted, and to the rest I think we can create the tickets to adjust further (paddings to other elements and sizes for example).

Screenshot 2025-01-21 at 09 42 47

Screenshot 2025-01-21 at 09 37 11

Screenshot 2025-01-21 at 09 33 15

Screenshot 2025-01-21 at 10 22 33

Screenshot 2025-01-21 at 10 20 32

Screenshot 2025-01-21 at 10 46 28

chiachenglu
chiachenglu requested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@chiachenglu chiachenglu left a comment

Choose a reason for hiding this comment

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

Please fix the above points before closing the PR.

mzedel added 7 commits January 21, 2025 22:43
@mzedel
fix(gui): layout adjustments to re-align UI after switch to outlined …
b0f8cc0 
…inputs

Ticket: None
Changelog: None
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel
chore(gui): migrated away from deprecated mui props + usetheme hook
2dbff75 
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel
test(gui): adjusted tests to work with changed labels for outlined te…
1e5d916 
…xt inputs

Ticket: None
Changelog: None
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel
chore(gui): aligned snapshots after input variant changes
8f79fc0 
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel
feat(gui): aligned updated billing section with bordered text input a…
176d13f 
…s well

Ticket: None
Changelog: None
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel
fix(gui): prevented sso config retrieval on plans that don't support …
bb292da 
…this

Ticket: None
Changelog: Title
Signed-off-by: Manuel Zedel <[email protected]>
@mzedel
fix(gui): prevented some warnings from rfh bleeding into the dom
06b5e89 
Ticket: None
Changelog: None
Signed-off-by: Manuel Zedel <[email protected]>
@mender-test-bot
Copy link
Contributor

mender-test-bot commented Jan 21, 2025 •
edited by jira bot
Loading

Merging these commits will result in the following changelog entries:

Changelogs

mender-server (men-7838)

New changes in mender-server since main:

Bug Fixes
  • prevented sso config retrieval on plans that don't support this
Features
  • aligned text input appearance with MUI updated guidelines
    (MEN-7838)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chiachenglu chiachenglu chiachenglu requested changes

@aleksandrychev aleksandrychev aleksandrychev left review comments

@michaelntech michaelntech Awaiting requested review from michaelntech

@mineralsfree mineralsfree Awaiting requested review from mineralsfree

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mzedel @chiachenglu @michaelntech @mender-test-bot @aleksandrychev