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

Widen alt style element #344

Merged
merged 5 commits into from
Feb 13, 2024
Merged

Widen alt style element #344

merged 5 commits into from
Feb 13, 2024

Conversation

Fweddi
Copy link
Contributor

@Fweddi Fweddi commented Feb 8, 2024

What does this change?

Before:
image

After:
image

We want more real estate for this type of element as it is closer to a document level item than an element level item. It also helps us deal with fitting our nested elements inside this element.

We now omit the headings for these fields because we want it to look closer to a document. We should consider adding better placeholders for these fields in a future PR (not trivial because placeholder logic is a bit of a muddle).

@Fweddi Fweddi requested a review from a team as a code owner February 8, 2024 10:34
Copy link
Contributor

@twrichards twrichards left a comment

Choose a reason for hiding this comment

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

LGTM, left a few comments?

I presume placing the controls above & below instead will be in a follow-up PR?

Any chance we could increase the left padding inside the inputs and increase the negative margin so the boxes are actually full width but text still aligns with stuff outside the repeater? (perhaps ignore if this doesn't affect composer, because no outer grey border) to essentially avoid this uncomfortable alignment on the left...
image

src/elements/alt-style/AltStyleElementForm.tsx Outdated Show resolved Hide resolved
src/elements/alt-style/AltStyleElementForm.tsx Outdated Show resolved Hide resolved
@Fweddi
Copy link
Contributor Author

Fweddi commented Feb 8, 2024

LGTM, left a few comments?

I presume placing the controls above & below instead will be in a follow-up PR?

Any chance we could increase the left padding inside the inputs and increase the negative margin so the boxes are actually full width but text still aligns with stuff outside the repeater? (perhaps ignore if this doesn't affect composer, because no outer grey border) to essentially avoid this uncomfortable alignment on the left... image

Yes, I want to address the nested controls in a follow-up PR. I think I see what you mean about the alignment, I'll have a look into this.

@Fweddi Fweddi merged commit 0414a91 into main Feb 13, 2024
3 checks passed
@Fweddi Fweddi deleted the fp/wider-alt-style-element branch February 13, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants