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

PTNFLY-938 Adding Find to the toolbar#383

Merged
jeff-phillips-18 merged 1 commit intopatternfly:masterfrom
junezhang:PTNFLY-938-Find
Aug 3, 2016
Merged

PTNFLY-938 Adding Find to the toolbar#383
jeff-phillips-18 merged 1 commit intopatternfly:masterfrom
junezhang:PTNFLY-938-Find

Conversation

@junezhang
Copy link
Copy Markdown
Member

https://patternfly.atlassian.net/browse/PTNFLY-938

Adding Find to the toolbar
Based on Jeff and Sarah’s feedback, refine the code.

@jeff-phillips-18
Copy link
Copy Markdown
Member

👍

@jeff-phillips-18
Copy link
Copy Markdown
Member

jeff-phillips-18 commented Jul 29, 2016

@dlabrecq @srambach Please review

@jeff-phillips-18
Copy link
Copy Markdown
Member

@dtaylor113 Please review

.btn{
outline: none;
}
.find-pf-dropdown-container{
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.

Can these -pf- classes be un-nested to reduce specificity without breaking anything? (.toolbar-pf-view-selector, .toolbar-pf-find, and .find-pf-dropdown-container?)

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.

@srambach, Thanks for checking. :-)
But .toolbar-pf-find and .find-pf-dropdown-container should be nested.
As the attached link: https://devops-ucd.rhcloud.com/June/patternfly/find.png

The position of .find-pf-dropdown-container is based on the father node .toolbar-pf-find.
Absolute positioning inside relative positioning make the .find-pf-dropdowm-container to be below of the magnifying glass icon.

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.

Sorry, I mean un-nesting the Less rules. The HTML is fine as is, but even if the elements are nested in the HTML, the selectors to style them don't need to be nested. Nesting them makes the selector very long and specificity increases, making it difficult to override the rules later if desired.
I think that you can move .toolbar-pf-view-selector, .toolbar-pf-action-right, .toolbar-pf-view-selector, and .toolbar-pf-find all out to the top level and everything still works fine.

@dtaylor113
Copy link
Copy Markdown
Member

Cloned this dev branch and ran the toolbar test page. Find LGTM.
Just need to address @srambach 's comments

@LHinson
Copy link
Copy Markdown
Member

LHinson commented Aug 2, 2016

@jeff-phillips-18 came across the note that "In the angular version, an interim solution was created that added a class to the toolbar when there are no filters and put in a bottom margin... base PF needs to address this so that the angularjs story can remove and replace with the base pf solution." Does this PR include this detail?

@junezhang
Copy link
Copy Markdown
Member Author

@jeff-phillips-18 @LHinson
I knew what you mean. I just think no need to add a class, I only add “margin-bottom: 10px” to .toolbar-pf-action. It can resolve the problem when no filters.
As the margin-top and margin-bottom of two elements will overlap, not conflict.
Please check if is it okay?

@jeff-phillips-18
Copy link
Copy Markdown
Member

@LHinson Yes, adding the 10px bottom-margin to .toolbar-pf-actions fixes the issue caused when there is no filtering. Angular-Patternfly can remove the code that works around this.

@LHinson
Copy link
Copy Markdown
Member

LHinson commented Aug 3, 2016

great, thank you @jeff-phillips-18 @junezhang

Looks like the comment from @srambach is the only open issue?

@junezhang
Copy link
Copy Markdown
Member Author

@srambach Thanks, I got your point, I will update the code soon.

@srambach
Copy link
Copy Markdown
Member

srambach commented Aug 3, 2016

Looks great! Thanks for the changes.

@jeff-phillips-18 jeff-phillips-18 merged commit 1d6e6b0 into patternfly:master Aug 3, 2016
andresgalante added a commit that referenced this pull request Aug 29, 2016
Review of PTNFLY-938 Adding Find to the toolbar #383
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.

5 participants