fix(pricing-table): cell items were being horizontally squished into … #11002
Conversation
|
Deploy preview created for package Built with commit: f945d7d7f8216cf108de0a000b2774d203be8aea |
|
Deploy preview created for package Built with commit: f945d7d7f8216cf108de0a000b2774d203be8aea |
|
Deploy preview created for package Built with commit: f945d7d7f8216cf108de0a000b2774d203be8aea |
|
Deploy preview created for package Built with commit: f945d7d7f8216cf108de0a000b2774d203be8aea |
jkaeser
added
feature flag
package: web components
There was a problem hiding this comment.
Love it! Simple and effective.
What do you think about updating the Pricing Table stories to slot multiple elements into the table cells, i.e. reflecting the conditions the bug appeared in? This would make it easier for other reviewers to test (I had to manually make changes to the DOM to see the fix in action), and it would help us catch any regressions in the future.
|
@jkaeser |
|
Deploy preview created for package Built with commit: f945d7d7f8216cf108de0a000b2774d203be8aea |
There was a problem hiding this comment.
You know what, @bruno-amorim ? I just remembered why this was made into a flex container to begin with. There's an option to add an icon into the cell content (e.g. the green checkmark in the Storybook examples), and when that's present, user-entered text should appear appear next to the icon. Sorry for not remembering earlier!
I think the fix is going to be a little more complicated. We need to make sure the user-entered content is wrapped in a containing element (i.e. a <div> or similar) that isn't set to display: flex;, and that containing element needs to be a sibling of the optional icon, both of which need a parent flex container. Something like the following sketch code:
<!-- dds-pricing-table-cell's shadow-root markup -->
<div style="display: flex;">
<div class="optional-icon"></div>
<div class="user-content">
<slot></slot>
</div>
</div>The tricky part is going to be getting that to work with all the other existing features, such as annotations. Let me know if you want a hand reasoning through it.
|
Deploy preview created for package Built with commit: bd8d91aed8936b72fa693bd38da4b9f105c05271 |
There was a problem hiding this comment.
Nicely done, @bruno-amorim! I left one small note for potential cleanup, but this looks great.
packages/styles/scss/components/pricing-table/_pricing-table.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
- 1. There needs to be some margin between the body & tag and tag & link elements, they should not touch each other as they do in the current build. If you do not have a designer specing these spacings for you, I would suggest:
- 8px top padding above the
tagcomponent - 16px top padding above the
linkcomponent
- 2. The link is too large in hierarchy compared to the rest of the pricing table content. It looks like this is using the Carbon for IBM.com link with icon, but in this case it should use the default Carbon link to result in a 14px type size. See the CWC storybook example. I would also suggest that links within the pricing table such as these should be the
inlinelink style instead of thelink with icon
oliviaflory
added
the
Needs design approval
|
Hi @oliviaflory cc @jkaeser |
There was a problem hiding this comment.
@bruno-amorim Thanks for making those updates! There's still some adjustments needed, though. Can you:
- Remove the tags and CTA content
- Wrap the existing lorem ipsum text in a
<span> - Add another small amount of example text wrapped in another
<span>- Example text: "Donec metus dui, pharetra sit amet dui eu, posuere imperdiet leo. Mauris vel quam vitae sem tincidunt pharetra. Etiam luctus placerat orci, non ultricies augue."
I think that will get us enough representative content to ensure our bug is fixed, and it should also be acceptable from a design point of view.
Let me know if you have any questions!
packages/web-components/src/components/pricing-table/__stories__/pricing-table.stories.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@bruno-amorim @jkaeser I apologize for the delayed review, I was out sick last week.
The multiple paragraphs look good, thank you for cleaning up the examples and removing the tags from the inner rows & columns.
One thing I will point out is that the checkmarks were not originally intended to be used in tandem with text so that the user's eye doesn't have to zig-zag down the page.
Current build
Intended use
I am going to approve this so that you can start using the multi-paragraph, but would really prefer to not showcase the checkmark and text together in the storybook examples. If you could make an update to the storybook preview I would appreciate it!
oliviaflory
removed
the
Needs design approval
|
Deploy preview created for package Built with commit: bd8d91aed8936b72fa693bd38da4b9f105c05271 |
jkaeser
added
the
Ready to merge
|
Deploy preview created for package Built with commit: f945d7d7f8216cf108de0a000b2774d203be8aea |
|
Please re-target this PR to the |
kodiakhq
bot
removed
the
Ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kennylam
added
the
Ready to merge
…the cell
Related Ticket(s)
https://jsw.ibm.com/browse/ADCMS-3714
Description
In a Pricing Table, for the Table Row when the user choses to "Add Cell Content" for cells to contain multiple paragraphs, or combination of paragraph text, Tag Group tags, or Links, when the page is published the Pricing table is displaying this added cell content distributed horizontally squished into the cell. The Added Cell Content is displaying to the right of the initial cell content in the cell.
I've changed the flex direction to column inside of the cell contents so that all items will sit one below the other.