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

Input: Don't add margin-top when input has no Label #8540

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

ralvarezing
Copy link
Contributor

@ralvarezing ralvarezing commented Mar 31, 2024

Description

When a Label is not set, a margin-top is not set. This fixes that inputs with no label won't align with other items.
This removes the margin-top from input text variants, but instead incorporates a new class (mud-input-{variant}-with-label), that adds margin top only if the input has a Label set.

fixes #8262

image
image
image
image
image

Before this change:
image

How Has This Been Tested?

Added a Viewer test to have all inputs affected by this fix in one place

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added the bug Something does not work as intended/expected label Mar 31, 2024
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.54%. Comparing base (8518947) to head (0d7928d).
Report is 11 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8540      +/-   ##
==========================================
+ Coverage   89.28%   89.54%   +0.25%     
==========================================
  Files         411      411              
  Lines       11903    11836      -67     
  Branches     2357     2349       -8     
==========================================
- Hits        10628    10598      -30     
+ Misses        752      716      -36     
+ Partials      523      522       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ralvarezing ralvarezing marked this pull request as ready for review March 31, 2024 03:50
@danielchalmers
Copy link
Contributor

Nice one! Looks great from the light testing in my app.

cc @henon

@henon
Copy link
Collaborator

henon commented Apr 1, 2024

For completeness, can you post a before picture that shows the same MudStack with misaligned text fields?

@ralvarezing
Copy link
Contributor Author

For completeness, can you post a before picture that shows the same MudStack with misaligned text fields?

There, I also added a button so it's easier to see the difference:

Before this change:
image

Afterr this change:
image

@henon henon added breaking change v7 New major MudBlazor version labels Apr 2, 2024
@henon
Copy link
Collaborator

henon commented Apr 2, 2024

I have another question. When you have textfields with labels, textfields without labels and buttons they will still all align well?

@ralvarezing
Copy link
Contributor Author

ralvarezing commented Apr 2, 2024

I have another question. When you have textfields with labels, textfields without labels and buttons they will still all align well?

This is a great question, and you're right,
because they will have different heights, they won't align right away (if you take the bottom line as base for alignment).
image
image

You could accept it as expected behavior because "they will have different heights" and the dev has two main options to fix it:

Fix this aligning using baseline and not center (But if you look close, the radio, checkbox and button are not aligned now).
image

Or Fix it with a mt-4 in each input that has no label. Which could be a good option because intentionally you want to add an input with label and an input with no label aligned (different "styles" in the same row).
image


Alternativity, we could change the approach and add a negative margin to the label itself (never add the margin to the input), and they will always align. ![image](https://github.com/MudBlazor/MudBlazor/assets/40799354/1d1eb0f7-8fa4-4fbd-b9e4-69ca95cafce3)

But you get this kind of problems
image
Which could be solved by the dev with a margin top to the input or to the Stack (depending on how is structure the layout)
But still, I don't think it should be how any component should work. Is like an intentional bug that the dev should "fix" when using it.

@henon
Copy link
Collaborator

henon commented Apr 2, 2024

Ok, I think the solution we have now is the most reasonable of all the options.

Thanks @ralvarezing

@henon henon merged commit 9cd0827 into MudBlazor:dev Apr 2, 2024
4 checks passed
@henon henon mentioned this pull request Apr 2, 2024
@henon
Copy link
Collaborator

henon commented Apr 2, 2024

Added to v7.0.0 Migration Guide #8447

danielchalmers pushed a commit to danielchalmers/MudBlazor that referenced this pull request Apr 2, 2024
@danielchalmers
Copy link
Contributor

@ralvarezing I think this broke MudAutocomplete visually. Could you check on your end?

v7:

image

v6:

image

@ralvarezing
Copy link
Contributor Author

Oooh yeah I think I know what happened... i'll breake it down

Almost all the inputs use the MudInput to implement the "field" itself, this MudInput is the one that implements the margins.
But it does not have the access to the Label that has been set from the parent (ex. MudTextField). So, when trying to build the classes, this evaluation !string.IsNullOrEmpty(baseInput.Label) is always false.

This is were this comes in:
image

And has to be set in all the inputs that uses this MudInput... So preatty sure I missed some (like the AutoComplete).

I'll see to get that fixed and before opening a new PR, double check that I don't miss any other

@ralvarezing
Copy link
Contributor Author

ralvarezing commented Apr 10, 2024

That should do it,
Is a really small PR but, for what I've seen, this should complete all inputs

Of corse, let me know if there is any other

@mrDenning
Copy link

mrDenning commented May 16, 2024

@ralvarezing I think this broke MudAutocomplete visually. Could you check on your end?

v7:

image

v6:

image

This issue still exists in 7.0.0-preview3 with MudTextField

@ralvarezing
Copy link
Contributor Author

@ralvarezing I think this broke MudAutocomplete visually. Could you check on your end?
v7:
image
v6:
image

This issue still exists in 7.0.0-preview3 with MudTextField

Sorry, can you provide some example in a playground or where do you have this issue.
I am not being able to reproduce this in the tag for the release 7.0.0-preview3 or later commits

@danielchalmers
Copy link
Contributor

@ralvarezing Noticed this in the switches in this DataGrid examples:

https://dev.mudblazor.com/components/datagrid#visual-styling
image

Before:

https://mudblazor.com/components/datagrid#editing
image

Do you think it's related to this PR?

@danielchalmers
Copy link
Contributor

And the sorting example

image

@ralvarezing
Copy link
Contributor Author

ralvarezing commented Jun 3, 2024

And the sorting example

image

Thxs for the example.

I haven't had much time, but taking a look, yes this is the cause of this, specifically the removal of the margin-inline-end.
Wasn't found in the switch examples because there is extra spacing added in those examples (which hide the missing margin-inline-end).

I've created this small pr (literally 2 chars :D) so this gets fixed fast: #9102
But I'm maybe taking a look at this issue if you're interested #9009

@danielchalmers
Copy link
Contributor

Thanks @ralvarezing, glad it's an easy fix.

I would love if you could look at #9009! Got semi approval here #8988 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something does not work as intended/expected v7 New major MudBlazor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inputs: Margin top does not allow to align with other components
4 participants