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(mrc): dont render TileBlock optional label #15261

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tristanwagner
Copy link
Contributor

@tristanwagner tristanwagner commented Feb 3, 2025

ref: ISSUE-15260

Question Answer
Branch? develop
Bug fix? yes
New feature? no
Breaking change? no
Tickets Fix #15260
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Related

@tristanwagner tristanwagner self-assigned this Feb 3, 2025
@github-actions github-actions bot added the bug Something isn't working label Feb 3, 2025
@tristanwagner tristanwagner linked an issue Feb 3, 2025 that may be closed by this pull request
3 tasks
@tristanwagner tristanwagner changed the title fix(mrc): not render TileBlock optional label fix(mrc): dont render TileBlock optional label Feb 3, 2025
@tristanwagner tristanwagner marked this pull request as ready for review February 3, 2025 10:49
@tristanwagner tristanwagner requested review from a team as code owners February 3, 2025 10:49
tibs245
tibs245 previously approved these changes Feb 3, 2025
ghyenne
ghyenne previously approved these changes Feb 3, 2025
Comment on lines 13 to 18
<dl className="flex flex-col gap-1 m-0">
<dt>
<OdsText preset={ODS_TEXT_PRESET.heading6}>{label}</OdsText>
</dt>
{label && (
<dt>
<OdsText preset={ODS_TEXT_PRESET.heading6}>{label}</OdsText>
</dt>
)}
<dd className="m-0">{children}</dd>
</dl>
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: By definition, dd tag provides the description for the preciding term dt.
If you have a requirement to not display dt, then imo, we have to think of adding modes for this where you display datalist/text/link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated as we said earlier, keeping the dt anyway but not rendering the OdsText since it is the part that cause the unwanted space

Copy link
Contributor

@rjamet-ovh rjamet-ovh Feb 3, 2025

Choose a reason for hiding this comment

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

For actions items, it will be better to not use dl/dt/dd, and render something specific. (probably a div with margin and children)

ref: ISSUE-15260

Signed-off-by: Tristan WAGNER <[email protected]>
Copy link

sonarqubecloud bot commented Feb 3, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[manager-components]: <TileBlock /> label should not render if empty
6 participants