chore(info-card): clean up info card example #190
Conversation
Use better example image that has intrinsic demensions, re-structure example markup to improve samples
|
Also, I don't seem to have permissions to assign reviewers. Is there any way someone could turn this on for me? Thx! |
|
@seanforyou23 I've added as a member of the pf-ng maintainers team to address this issue! |
|
Can you please add a link to your rawgit example? A snapshot would also be helpful. |
|
As I shrink the window to the left, the card with jumps wide again. It appears the layout is using col-md-6, then jumps to 8 columns using col-sm-8. Before: After: |
address scenario where card width is too small which causes textual content to overflow the bounds of the card patternfly/patternfly-ng#190
|
@dgutride thanks for the feedback. Sorry I forgot to post a link to the rawgit! Here it is; https://rawgit.com/seanforyou23/patternfly-ng/info-card-example-dist/dist-demo/#/infocard For restricting the length of the card - this is impacted by the container that houses the card examples. Because the primary container uses the class "container-fluid", even the smallest practical large column class (col-lg-3) ends up with extra space when expanding the browser window to a point. Was able to get around this by simply setting a max-width on a parent container, this is essentially what's happening in the angular-patternfly example. Below is a screenshot of what it now looks like on large viewports.
Let me know if you find anything else that needs attention. Thanks!! |
There was a problem hiding this comment.
There is also an issue on the following class that is causing the content of your card to bleed out into the well:
.card-pf-info-status {
display: flex;
}
In order to prevent bleed-through, it should be updated to be
.card-pf-info-status {
display: inline-flex;
}
| <div class="padding-15"> | ||
| <div class="row row-cards-pf well"> | ||
| <div style="max-width:409px;"> | ||
| <div class="col-md-10"> |
There was a problem hiding this comment.
Change this to <div class="col-sm-8 col-md-7">
| </div> | ||
| <div class="padding-15"> | ||
| <div class="row row-cards-pf well"> | ||
| <div style="max-width:409px;"> |
There was a problem hiding this comment.
This is conflicting with the column width that you have set below. In the "medium" viewport, you are making the card smaller than the column wants it to be.
I would delete this line, and change the column styles on the next. (see below)
| <div class="cards-pf"> | ||
| <div class="row row-cards-pf"> | ||
| <div class="col-sm-4"> | ||
| <br> |
There was a problem hiding this comment.
Instead of using <br>, you can make each card it's own row. That will give proper spacing and allow for responsiveness on resizing of the viewport.
…h fluid and limited width element containers
…xample stylesheet
|
@mindreeper2420 thanks for pointing that out, I was actually struggling with confirming what was the best way of addressing the overflow. After speaking with a few other devs I realized I had an issue with the markup I was using which was complicating things. Setting the card to inline-flex ends up causing other layout issues so I wasn't able to use that method, however word-break solved it nicely and didn't affect the surrounding page elements. Anyway, it's ready for another review when you have time. Thx! |
| } | ||
| } | ||
| .cards-pf { | ||
| padding-bottom: 20px; |
There was a problem hiding this comment.
with View.Encapsulation: None;, this may pose a problem with styles being injected into the <head> of the site.
There was a problem hiding this comment.
It's a good point, thanks @mindreeper2420 - I've prefixed the example selectors with a class which is specific to the info status card example, this should keep any conflict from arising.
| padding-bottom: 20px; | ||
| } | ||
| .cards-pf .row-cards-pf { | ||
| margin-bottom: @bottomMargin; |
There was a problem hiding this comment.
same here with View Encapsulation
There was a problem hiding this comment.
Since this is a single page app, it only effects this particular card example, but we should use best practices.
We can add a level of specificity by applying the margins/padding to restrictive-width-example-container and fluid-width-example-container, for example.
…s with selector that is specific to info status card example
|
LGTM too! Thanks for making the updates! |
This pull request improves the display of the example for the info status card component. What was previously thought to be a core styling issue with the info card, was realized to be caused by an issue with the way the example markup was structured.
This may invalidate the need for adjusting the top/bottom margin at the core card component level - which is mentioned in a related ticket.