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

admin field width does not work (CSS specificity bug) #7867

Closed
brunocrosier opened this issue Aug 26, 2024 · 14 comments
Closed

admin field width does not work (CSS specificity bug) #7867

brunocrosier opened this issue Aug 26, 2024 · 14 comments
Assignees

Comments

@brunocrosier
Copy link
Contributor

brunocrosier commented Aug 26, 2024

Link to reproduction

No response

Payload Version

beta

Node Version

20

Next.js Version

beta

Describe the Bug

This line of CSS is explicitly setting width: 100%!important, meaning that when you do something like

admin: {
  width: '50%'
}

although the width: 50% is actually set on the element, the width: 100%!important wins out, and we can't have fields of custom width
the

Reproduction Steps

Create a collection and add

admin: {
  width: '50%'
}

to any field

### Adapters and Plugins

_No response_
@brunocrosier brunocrosier added status: needs-triage Possible bug which hasn't been reproduced yet v3 labels Aug 26, 2024
@brunocrosier brunocrosier changed the title width does not work admin field width does not work (CSS specificity bug) Aug 26, 2024
@github-actions github-actions bot removed the status: needs-triage Possible bug which hasn't been reproduced yet label Aug 26, 2024
@AlessioGr AlessioGr added status: cant-reproduce If an issue cannot be reproduced status: needs-repro If an issue does not include a reproduction labels Aug 26, 2024
@github-actions github-actions bot removed status: needs-repro If an issue does not include a reproduction status: cant-reproduce If an issue cannot be reproduced labels Aug 26, 2024
@AlessioGr
Copy link
Member

CleanShot 2024-08-26 at 13 15 59@2x

Can't reproduce this issue - works for me, see screenshot, Can you provide a reproduction / a field config where this issue happens?

@brunocrosier
Copy link
Contributor Author

ah yes i'm sorry it seems like the issue is actually something else, and only happens when the widths of a field inside a row add up to 100%

{
            type: 'row',
            fields: [
                {
                    name: 'SKU',
                    type: 'text',
                    admin: {
                        width: '50%',
                    }
                },
                {
                    name: 'taxRate',
                    type: 'relationship',
                    relationTo: 'taxes',
                    hasMany: false,
                    admin: {
                        width: '50%',
                    }
                },
            ]
        },

So actually, the fix should be to change from flex-grow: 1; to flex: 1; (which is shorthand for also adding flex-shrink: 1; flex-basis: 0%;

@brunocrosier
Copy link
Contributor Author

brunocrosier commented Aug 26, 2024

Created PR here: #7869

@GermanJablo
Copy link
Contributor

Hi @brunocrosier. I still cannot reproduce.

image

Also, if I change flex-grow to flex like you did in your PR, the relationship doesn't hold if it's uneven (say 10% and 40%), but instead, they keep taking up half of each other.

Maybe you have some CSS of your own that could be conflicting?

@brunocrosier
Copy link
Contributor Author

Hey @GermanJablo which version of payload v3 beta are you using?

@GermanJablo
Copy link
Contributor

I checked out your PR

@brunocrosier
Copy link
Contributor Author

Hmm.. I created my PR via the GitHub UI and I think it branched off of main rather than beta. That might be why..

I'm running 3.0.0-beta.94 and that's where I am seeing the issue

@tophermurphy
Copy link

I'm seeing the same issue on beta.94. It is because the css uses "gap". The gap adds space to the row children, so a row with 2 elements of 50% and a gap of 10px calculates out at 100% + 10px, thus overflowing the content.

Either switch "width" to "max-width", revert to the previous style with negative margins and padding or set the flex-wrap to nowrap.

Here is how the rows where handled in the previous version

.field-type.row .row__fields {
  display: flex;
  flex-wrap: wrap;
  width: calc(100% + var(--base));
  margin-left: calc(var(--base) * -0.5);
  margin-right: calc(var(--base) * -0.5);
}

.field-type.row .row__fields > * {
  flex-grow: 1;
  padding-left: calc(var(--base) * 0.5);
  padding-right: calc(var(--base) * 0.5);
}

@tophermurphy
Copy link

If you drop this in custom.scss file it has the widths working properly.

.field-type.row .row__fields {
    display: flex;
    flex-wrap: wrap;
    margin-left: calc(var(--base) * -0.5);
    margin-right: calc(var(--base) * -0.5);
    gap: 0;
  }

  .field-type.row .row__fields > * {
    flex-grow: 1;
    padding-left: calc(var(--base) * 0.5);
    padding-right: calc(var(--base) * 0.5);
  }

@GermanJablo
Copy link
Contributor

@brunocrosier Do you want to update your PR to point to the Beta branch? The new path if I'm not mistaken is:
packages\ui\src\fields\Row\index.scss

Ideally, it would be good to have a test. If that's not possible, I would appreciate if you could do some verifications that flex: 1 doesn't cause any undesired behavior.


@tophermurphy thank you for your contribution. Let's first see if @brunocrosier 's solution fixes the problem, since it only changes 1 line of CSS

@brunocrosier
Copy link
Contributor Author

@brunocrosier Do you want to update your PR to point to the Beta branch? The new path if I'm not mistaken is: packages\ui\src\fields\Row\index.scss

Ideally, it would be good to have a test. If that's not possible, I would appreciate if you could do some verifications that flex: 1 doesn't cause any undesired behavior.

@tophermurphy thank you for your contribution. Let's first see if @brunocrosier 's solution fixes the problem, since it only changes 1 line of CSS

Thanks I'll create a new PR pointing at beta and add some tests. Just pinged you on DIscord with some questions about how to run Payload in the beta branch. Cheers

@brunocrosier
Copy link
Contributor Author

@brunocrosier Do you want to update your PR to point to the Beta branch? The new path if I'm not mistaken is: packages\ui\src\fields\Row\index.scss

Ideally, it would be good to have a test. If that's not possible, I would appreciate if you could do some verifications that flex: 1 doesn't cause any undesired behavior.

@tophermurphy thank you for your contribution. Let's first see if @brunocrosier 's solution fixes the problem, since it only changes 1 line of CSS

I've addressed this issue in this PR now and added some more tests: #7940

Reviews welcome!

GermanJablo added a commit that referenced this issue Sep 11, 2024
Closes #7867

Problem: currently, setting an 

```ts
admin: {
   width: '30%'
}
```

does not work for fields inside a row or similar (group, array etc.)

Solution: when we render the field, we set a CSS variable
`--field-width` with the value of `admin.width`. This allows us to
calculate the correct width for a field in CSS by doing `flex: 0 1
var(--field-width);`

It also allows us to properly handle `gap` with `flex-wrap: wrap;`

Notes: added playwright tests to ensure widths are correctly rendered


![image](https://github.com/user-attachments/assets/0c0f11fc-2387-4f01-9298-a2613fceee22)
Copy link
Contributor

🚀 This is included in version v3.0.0-beta.104

Copy link
Contributor

This issue has been automatically locked.
Please open a new issue if this issue persists with any additional detail.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants