Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Fixed the border problem in list-view page#444

Closed
junezhang wants to merge 1 commit intopatternfly:masterfrom
junezhang:list-view-border
Closed

Fixed the border problem in list-view page#444
junezhang wants to merge 1 commit intopatternfly:masterfrom
junezhang:list-view-border

Conversation

@junezhang
Copy link
Copy Markdown
Member

Description

Fixed this bug for when selected a row in list view page, only the top blue border will display.

Reference this link: patternfly/patternfly#434 (comment)

Comment thread less/list-view.less Outdated
&.active:first-child {
border-top: 1px solid @list-group-active-border;
margin-top: -1px;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't believe this block is necessary. It is using the same values as the non first-child active items.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because in list-group.less, we have this code:
.list-group {
border-top: 1px solid @list-group-top-border;
.list-group-item:first-child {
border-top: 0;
}
}
set the first child border top is 0, I need the top border, so add a 1px to top border.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But that does not effect .list-view-pf and does not come into play here

@junezhang
Copy link
Copy Markdown
Member Author

@jeff-phillips-18 Thanks, Jeff. Fixed it.

@dlabrecq
Copy link
Copy Markdown
Member

dlabrecq commented Sep 1, 2016

@jgiardino Wasn't this an intentional change? I'm looking at #434 and it states, "with this CSS removed, only a top blue border will display for selected items (for any row except first)."

@jgiardino
Copy link
Copy Markdown
Contributor

The update I checked in for #434 was to address the issue with additional borders displaying in the vertical nav that was introduced with #400. #400 did not provide details about why border styles for list groups were modified, and I didn't see details anywhere else that stated what the expected visual design was for list view borders, so for #434 I pulled out the css that caused the issue since it was unrelated to the actual issue stated in #400.

I don't have a strong opinion on the visual design for the borders, and am interested in what the rest of the design team thinks this should be.

However, there are two points I'd like to make related to this change:

  1. When changes are made, then the PR needs to document what changes were made and the history behind those changes (or mention the JIRA story) so that other team members are aware of what is changing and why.
  2. For optimal performance, we should avoid applying styles during user interaction (e.g. hover, click) that affect layout (e.g. border-width, margin) when we can achieve the same effect by only modifying styles that do not affect layout (e.g. border color). I included more comments related to this in Removing css that caused issue with borders displaying in secondary a… #434.

@jgiardino
Copy link
Copy Markdown
Contributor

@dlabrecq Please review latest comments posted to #434

@dlabrecq
Copy link
Copy Markdown
Member

dlabrecq commented Sep 1, 2016

It wasn't clear if the border should be there or not? And if this change will have any affect the navigation menu? It looked to me that the top blue border in the navigation menu was removed intentionally.

I just wanted to ensure that adding this border back in won't override what you were trying to achieve with #434. If the change looks good to you, I'm ok with it.

@junezhang
Copy link
Copy Markdown
Member Author

@jgiardino Thanks for your great suggestions.
Based on the comments from #434, visual design team suggested to use the single border.
So close this PR.

@junezhang junezhang closed this Sep 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants