Bug 1866337: Changes placeholder to 'Search by node name/label' in install page #6746
Conversation
openshift-ci-robot
added
bugzilla/severity-medium
|
@vbnrh: This pull request references Bugzilla bug 1866337, 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
the
component/core
| filterPlaceholder || (FilterType.NAME === filterType ? 'Search by name...' : 'app=frontend'); | ||
| const [placeholder, setPlaceholder] = React.useState( | ||
| FilterType.NAME === filterType | ||
| ? `Search by ${kind !== undefined ? kind.toLowerCase() : ''} name...` |
There was a problem hiding this comment.
I would just lowercase kind. It is awkward for longer names since the word boundaries are no longer apparent.
There was a problem hiding this comment.
The good reasons for using this approach and not just using nameFIlterPlaceholder :-
- We won't have to hardcode the resource ( Node ) and the type of filter (Name, Label)
- filter-toolbar being a generic component can accommodate multiple types of filter in the future. This could handle those scenarios too.
- We just can pass the type of resource by kind prop and let the filter automatically handle any resource types.
There was a problem hiding this comment.
I would just lowercase kind. It is awkward for longer names since the word boundaries are no longer apparent.
Sorry i didn't get this point. Could you explain a bit more ?
There was a problem hiding this comment.
You shouldn't just manipulate kind with toLowercase. Things like SecurityContextContraint become securitycontextconstraint which is hard to read. You would have to use the model display name.
Won't this change all the list pages instead of the one we want to change?
There was a problem hiding this comment.
Yes i understand your concern on readability in which case we may need to keep non lower cased name or pass a seaparate prop for the resourceName :)
Won't this change all the list pages instead of the one we want to change?
Yes that was the initial idea that if we are not passing the kind prop, it will just show 'Search by name' instead of 'Search by {kind} name'
|
Hi, we're only a few days away from code freeze for 4.6, and this change affects all list pages in the console. Can we go with a simpler fix like
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
vbnrh
force-pushed
the
filter-by-name-label
branch
from
2b27eca
to
4ee0bfe
Compare
Signed-off-by: Vineet Badrinath <vbadrina@redhat.com>
vbnrh
force-pushed
the
filter-by-name-label
branch
from
4ee0bfe
to
7b8f927
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spadgett, vbnrh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
lgtm
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@vbnrh: All pull requests linked via external trackers have merged: Bugzilla bug 1866337 has been moved to the MODIFIED state. 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. |
This PR add fixes to previous #6621. Now we can pass a resource name to display the placeholder appropriately and it will change the placeholder according to the type (Name, Label etc) selected
Signed-off-by: Vineet Badrinath vbadrina@redhat.com