Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorporate vertical scroll shadows to modals #1128

Merged

Conversation

sg00dwin
Copy link
Member

  • shared css for react-modal and patternfly-react modal
  • modal-content class is now a child of modal-dialog
  • pf overrides for catalog height and positioning to support scroll shadows
  • button, input, tag elements have colors switched to rgba so don't overlay shadow

modals-scroll-shadows
dev-catalog-scroll-shadows
android-scroll-shadows
ios-dev-catalog-scroll-shadows

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2019
@sg00dwin sg00dwin force-pushed the modal-vertical-scroll-shadows branch from 399bdd5 to a65c6bb Compare January 23, 2019 20:18
@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

/refresh
/skip

@spadgett
Copy link
Member

/assign @rhamilto

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

Nice job moving us toward consolidated modals and improving their funcitonality!


// Modal modifications to enable vertical scrolling with shadow overlays

.modal-dialog {
Copy link
Member

Choose a reason for hiding this comment

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

NIt: alpha


.modal-dialog {
@media(max-width: $screen-xs-max) {
height: calc(100% - 20px);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment explaining why 20px?

}

}
.btn-default {
Copy link
Member

Choose a reason for hiding this comment

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

This gives me pause since it's redefining the buttons style from PatternFly. If/when those change, we will now have to update this rule as well.



.modal.right-side-modal-pf {
top: 76px; // since PatternFly 4's masthead is taller than PatternFly 3's
Copy link
Member

Choose a reason for hiding this comment

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

Nit: indentation.

top: 54px;
width: auto;
&.modal-body--centered {
@media(min-width: $screen-sm-min) {
Copy link
Member

Choose a reason for hiding this comment

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

These changes seem undesirable for short modals?

screen shot 2019-01-28 at 10 09 56 am

Copy link
Member Author

Choose a reason for hiding this comment

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

screen shot 2019-01-30 at 12 02 52 pm

@@ -43,7 +43,7 @@ class DeleteModal extends PromiseComponent {

render() {
const {kind, resource} = this.props;
return <form onSubmit={this._submit} name="form">
return <form onSubmit={this._submit} name="form" className="modal-content">
<ModalTitle>Delete {kind.label}</ModalTitle>
<ModalBody className="modal-body co-delete-modal">
Copy link
Member

Choose a reason for hiding this comment

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

Note the passing of className here overrides the default classNames in modal.tsx. As a result, this modal doesn't get the height adjustments. Is that intentional?

@@ -28,7 +28,7 @@ class DeleteNamespaceModal extends PromiseComponent {
}

render() {
return <form onSubmit={this._submit} name="form">
return <form onSubmit={this._submit} name="form" className="modal-content">
<ModalTitle>Delete {this.props.kind.label}</ModalTitle>
<ModalBody className="modal-body co-delete-modal">
Copy link
Member

Choose a reason for hiding this comment

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

Note the passing of className here overrides the default classNames in modal.tsx. As a result, this modal doesn't get the height adjustments. Is that intentional?

@sg00dwin sg00dwin force-pushed the modal-vertical-scroll-shadows branch 2 times, most recently from 6398423 to 4190d6a Compare January 30, 2019 17:01
@sg00dwin
Copy link
Member Author

@rhamilto changes push. please take a look.

size options
screen shot 2019-01-30 at 11 57 34 am
screen shot 2019-01-30 at 11 57 54 am
screen shot 2019-01-30 at 11 58 04 am

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

/lgtm

@@ -26,42 +26,76 @@
}
}


Copy link
Member

Choose a reason for hiding this comment

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

Nit: extra blank line

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019
- modal-content class is now a child of modal-dialog
- shared css for react-modal and patternfly-react modal
- add modal optional height classes
- pf overrides for catalog height and positioning to support scroll shadows
@sg00dwin sg00dwin force-pushed the modal-vertical-scroll-shadows branch from 4190d6a to 415a38c Compare January 30, 2019 18:04
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019
@rhamilto
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019
@openshift-merge-robot openshift-merge-robot merged commit 1847490 into openshift:master Jan 30, 2019
@sg00dwin sg00dwin deleted the modal-vertical-scroll-shadows branch January 31, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants