Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

[terra-clinical-label-value-view] Fix margin for dl elements #904

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

smason0
Copy link
Contributor

@smason0 smason0 commented Aug 17, 2023

Summary

What was changed:
Removed top and bottom margins added by the default stylesheet of the <dl> element within the LabelValueView element (the wrapper element when isChildOfDescriptionList prop is 'false' or undefined).

Why it was changed:
These margins were added unintentionally when a definition list was added to this component as part of a11y changes, and this change fixes that by removing them.

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Component example (before/after):
Screenshot 2023-08-23 at 3 55 24 PM
Screenshot 2023-08-23 at 3 55 50 PM
Multiple adjacent LabelValueViews (before/after):
Screenshot 2023-08-23 at 4 04 22 PM
Screenshot 2023-08-23 at 4 04 41 PM

Note: Some components, like DetailListItem, add their own padding to separate each ListValueView, as seen below
Screenshot 2023-08-23 at 5 04 23 PM

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-9469


Thank you for contributing to Terra.
@cerner/terra

@smason0 smason0 changed the title [terra-clinical-label-value-view] Fix padding for dl elements [terra-clinical-label-value-view] Fix margin for dl elements Aug 23, 2023
@smason0 smason0 self-assigned this Aug 24, 2023
@smason0 smason0 marked this pull request as ready for review August 24, 2023 13:40
Comment on lines +12 to +15
#dl-wrapper {
margin-bottom: 0;
margin-top: 0;
}
Copy link

Choose a reason for hiding this comment

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

Why do we need to add a style on the wrapper element if we are updating our sass file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is being applied to the <dl> element on line 19 for this specific example and not the one in LableValueView that had its sass file updated. This change is maybe not needed, as it is just an example, but I added it to be consistent in removing these default margins whenever we use a definition list.

@sdadn
Copy link
Contributor

sdadn commented Aug 24, 2023

How do these changes compare to the original styling utilizing div elements? Are the margins same or different? etc.

@smason0
Copy link
Contributor Author

smason0 commented Aug 24, 2023

How do these changes compare to the original styling utilizing div elements? Are the margins same or different? etc.

The changes make it appear the same as when the wrapper is a div, i.e. the top and bottom margins are 0.

div:
Screenshot 2023-08-24 at 1 29 48 PM

dl:
Screenshot 2023-08-24 at 1 31 28 PM

@github-actions github-actions bot temporarily deployed to preview-pr-904 August 29, 2023 13:59 Destroyed
@sdadn sdadn merged commit 0b46a98 into main Aug 29, 2023
21 checks passed
@sdadn sdadn deleted the UXPLATFORM-9469 branch August 29, 2023 17:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants