Bug 1830312: Remove Filter button - Search Page #5346
Conversation
|
@zherman0: This pull request references Bugzilla bug 1830312, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
bugzilla/severity-unspecified
openshift-ci-robot
added
component/core
|
@zherman0: This pull request references Bugzilla bug 1830312, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/severity-low
|
@bmignano @benjaminapetersen - Ready for review |
|
/retest docker image issue - retesting |
|
@bipuladh @jeff-phillips-18 - Adding you guys since you were recently working in this code |
|
/retest |
|
Developer perspective for UX review:
@openshift/team-devconsole-ux |
|
This is looking great, thanks Zac! Not sure about the alignment of the pin link and button on mobile view for dev side though. Maybe they should all be left aligned. Thoughts @beaumorley ? Otherwise, LGTM! |
|
Spoke with @jeff-phillips-18 and saw that the two buttons will overlap on Dev side before the nav hides at tablet screen size. Can we bump both of the buttons down so they are still inline but underneath and left-aligned with the title at that point? And when the nav hides, we can move them back inline with the title? We'd do the same thing on the admin side @zherman0 with the "create" button. @jeff-phillips-18 On mobile, when those buttons start overlapping again, can we combine them into a kebab? Hopefully that would allow it to also sit on the same line as the title. @zherman0 We'd want the Create button to continue to fall below the title and be left-aligned. @bmignano @beaumorley Please let me know your thoughts on this! |
|
@gdoyle1 @jeff-phillips-18 - Let's do the dev-console mobile alignment and the kebab work as a following on bug or feature in 4.6. |
zherman0
force-pushed
the
search2
branch
2 times, most recently
from
d1b6252
to
b545382
Compare
|
/test images |
|
/retest 503 Service Unavailable error |
|
/assign @benjaminapetersen |
We need to do something with mobile layout in 4.5 or defer the entire change since it looks broken. Not necessarily a kebab, but something. We shouldn't knowingly introduce a layout bug in 4.5. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zherman0 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@spadgett Can we left align like @gdoyle1 suggested above for tablet views? I think it could be a fine interim approach for mobile as well. |
zherman0
changed the title
|
@zherman0: This pull request references Bugzilla bug 1830312, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
3 similar comments
|
@zherman0: This pull request references Bugzilla bug 1830312, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@zherman0: This pull request references Bugzilla bug 1830312, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@zherman0: This pull request references Bugzilla bug 1830312, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@spadgett - I updated this PR to simply remove the filter button that shows on ListPages when displayed in the search results. I will open another PR to move the |
|
@zherman0: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
This would not fix https://bugzilla.redhat.com/show_bug.cgi?id=1835348
https://github.com/openshift/console/pull/5346/files#diff-250131ae4d03b64ce9bae143cec0b46aL295
This line is not supporting hideNameFilter. We initially hid it when this was set. The recent move to DataToolbar probably lost this prop being applied there.
If we could make it something like
chips={nameFilter && && !hideNameFilter && nameFilter.length > 0 ? [nameFilter] : []}
The issue pointed out in the BZ would go away.
| @@ -79,6 +80,7 @@ export class ListPageWrapper_ extends React.PureComponent { | |||
| textFilter={textFilter} | |||
| hideNameFilter={hideNameFilter} | |||
| hideLabelFilter={hideLabelFilter} | |||
| hideRowFilter={hideRowFilter} | |||
There was a problem hiding this comment.
| hideRowFilter={hideRowFilter} | |
| rowFilters={[]} |
I don't find it necessary for us to add a new prop.
There was a problem hiding this comment.
However, this requires modification in several places as list pages override rowFilters prop.
We could also pass in filters="HIDE_FILTER" so that the row filters get overridden internally.
There was a problem hiding this comment.
I reopened bz1835348 and we will fix that in a different PR.
There was a problem hiding this comment.
I have raised a PR for 1835348 #5481. However, I think we should consider using filters props rather than introducing a new prop. :)
| @@ -106,6 +108,7 @@ ListPageWrapper_.propTypes = { | |||
| customData: PropTypes.any, | |||
| hideNameFilter: PropTypes.bool, | |||
| hideLabelFilter: PropTypes.bool, | |||
| hideRowFilter: PropTypes.bool, | |||
There was a problem hiding this comment.
| hideRowFilter: PropTypes.bool, |
|
@bipuladh - do you think your PR5481 is going to make this PR obsolete? I would love to just have the one hideToolBar property if it will hide the filter row as well. |
Yes @zherman0 5481 will make this PR obsolete. Since we are removing |
|
Replaced by #5481. We'll look at updating the Create button separately. That's a larger change. |
|
@spadgett: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Removed the filter button in results section of the search. BZ1830312
Now:
Before: