Bug 1997482: Remove backdrop from Pipeline tasks search modal #9880
Conversation
vikram-raj
changed the title
openshift-ci
bot
added
the
bugzilla/severity-low
|
@vikram-raj: This pull request references Bugzilla bug 1997482, 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
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request. 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
bot
added
the
bugzilla/valid-bug
|
/kind bug |
|
/cc @karthikjeeyar |
|
cc: @bgliwa01 |
...packages/pipelines-plugin/src/components/pipelines/pipeline-builder/PipelineBuilderForm.scss
Outdated
Show resolved
Hide resolved
|
/approve |
|
/hold |
| .pf-c-backdrop { | ||
| background: none; | ||
| } |
There was a problem hiding this comment.
We already have this solution in Topology. This is not the right way to address this. We need one solution for both places. Please move this into the Quick Search and address it there and remove it from TopologyView.scss.
There was a problem hiding this comment.
We do not have a common wrapper for pf-c-backdrop that is why we have to set it at two places. If we set background for pf-c-backdrop without nesting. Then it will affect all the modals we have.
There was a problem hiding this comment.
Can you not wrap it? Or provide a className to the Modal from PF? Surely there are ways to bring this into the component @vikram-raj
There was a problem hiding this comment.
Added a common class to the quick search modal and defined that class in QuickSearchModal component.
frontend/packages/console-shared/src/components/quick-search/QuickSearchModalBody.scss
Outdated
Show resolved
Hide resolved
openshift-ci
bot
added
do-not-merge/hold
|
/remove-approve |
|
/lgtm cancel |
|
/remove-approve |
openshift-ci
bot
removed
the
approved
| <div ref={contentRef} className="odc-pipeline-builder-form"> | ||
| <div | ||
| ref={contentRef} | ||
| className="odc-pipeline-builder-form ocs-quick-search-modal__no-backdrop" |
There was a problem hiding this comment.
Instead of having this classname ocs-quick-search-modal__no-backdrop in PipelineBuilderForm.tsx and TopologyView.tsx, can we have this directly in the quickSearchModal.tsx. Maybe a configuration prop (backdropEnabled) that decides if the backdrop classname has to be added or not (default can be a false). WDYT?
quickSearchModal.tsx
<Modal
className={cx('ocs-quick-search-modal', {
'ocs-quick-search-modal__no-backdrop': !backdropEnabled})} // defaults to false
...
>
There was a problem hiding this comment.
I don't think this is needed, from my comment:
I'm adding LGTM as I don't think @karthikjeeyar's suggestion about classNames usage will add much if we need to return to it.
There was a problem hiding this comment.
Yeah, we can skip it.
There was a problem hiding this comment.
/lgtm
Log a technical debt ticket @vikram-raj We need to move away from this PF modal if it's not going to do our job. This is the 2nd issue we have where the PF modal is needing to be overridden in some fashion.
The other is when the drag-n-resize stuff happens, we are actually moving the body and not the modal. Since the modal inserts itself into the viewContainer we don't have access to styling it. The styles for the Modal are past the backdrop component so it can't style it.
Currently today the only way to make this happen in one spot would be to use a useEffect and remove the class / modify the styles through a JS grab of the dom element. Which is prone to it's own problems and likely would cause issues.
I'm adding LGTM as I don't think @karthikjeeyar's suggestion about classNames usage will add much if we need to return to it.
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
/approve |
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, bgliwa01, karthikjeeyar, vikram-raj 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 |
|
@vikram-raj: All pull requests linked via external trackers have merged: Bugzilla bug 1997482 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. |
Fixes:
https://issues.redhat.com/browse/ODC-6277
Analysis / Root cause:
Modal backdrop should not be present as per the design.
Solution Description:
Screen shots / Gifs for design review:
Browser conformance: