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

fix(forms): updates Message to receive margin when intermediaries are present #1799

Merged
merged 4 commits into from May 1, 2024

Conversation

geotrev
Copy link
Contributor

@geotrev geotrev commented Apr 29, 2024

Description

Implements a general sibling combinator for Messages in a Field. This way an Input andMessage can have intermediary elements but the latter can still space correctly (like in the case of DatePicker).

Detail

Currently, DatePicker doesn't hvae any spacing when a Message is present. This happens because the menu wrapper sits between them, and the existing adjacent sibling combinator invalidates this scenario:

Screenshot 2024-04-29 at 1 32 05 PM

Compare to Input, which has the correct margin-top when a Message is used:

Screenshot 2024-04-29 at 1 29 22 PM

Now using the general sibling combinator:

Screenshot 2024-04-29 at 1 32 15 PM

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

</Datepicker>
</Field>
<Col alignSelf="center">
<div style={{ display: 'flex', justifyContent: 'center' }}>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed. I think you can apply justifyContent to the Row instead. Compare

<Grid>
<Row justifyContent="center" style={{ height: 'calc(100vh - 80px)' }}>
<Col alignSelf="center">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 👍🏻 I looked at the Col props and missed that prop for Row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lookin good I think. Removed the hardcoded width so the input could adjust with the column.

@geotrev geotrev changed the title 🚧 fix(forms): updates Message to receive margin when intermediaries are present fix(forms): updates Message to receive margin when intermediaries are present Apr 29, 2024
@geotrev geotrev marked this pull request as ready for review April 29, 2024 22:02
Comment on lines 30 to 34
validation: {
options: ['success', 'warning', 'error'],
control: { type: 'radio' },
table: { category: 'Message' }
},
Copy link
Member

Choose a reason for hiding this comment

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

Let's restructure this to behave more like Input or Combobox

  • default the message children to "Message"
  • add a Story category section control for toggling the Message on
  • move validation to an Input category section and allow it to control validation styling for both the input and the message together (as there is no design recommendation for seeing them happen separately)

<Row style={{ height: 'calc(100vh - 80px)' }}>
<Col textAlign="center" alignSelf="center">
<Row justifyContent="center" style={{ height: 'calc(100vh - 80px)' }}>
<Col alignSelf="center" xs={12} md={4}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Col alignSelf="center" xs={12} md={4}>
<Col textAlign="center" alignSelf="center">

...let's avoid the change in order to remain consistent with all other centered non-responsive dropdown type stories

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It's more consistent (compared to what this demo looked like before, which I was attempting to recreate) so I'm all for it.

},
validationLabel: {
control: { type: 'text' },
table: { category: 'Input' }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
table: { category: 'Input' }
table: { category: 'Message' }

[nit] the validationLabel actually belongs to the Message's icon. See https://garden.zendesk.com/components/input#message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, shoot, I see it now in the Input story too. Fixing.

@geotrev geotrev merged commit 9b59a96 into main May 1, 2024
5 checks passed
@geotrev geotrev deleted the george/dp-input-message branch May 1, 2024 15:07
ze-flo pushed a commit that referenced this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants