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.

Sign up for GitHub

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

Vendor review approval modal content edit and close button fix #1206

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Vendor review approval modal content edit and close button fix #1206

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
576505f
Modify vendor review message
stalgiag Aug 28, 2024
dde41e4
Ensure ProvideFeedbackModal can close
stalgiag Aug 28, 2024
201e820
Different icon for NotApprovedModal
stalgiag Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions ...w/CandidateModals/ThankYouModal/index.jsx → ...w/CandidateModals/ApprovedModal/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,41 @@ import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faCheck } from '@fortawesome/free-solid-svg-icons';
import PropTypes from 'prop-types';
import '../common.css';
import './ThankYouModal.css';

const ThankYouModal = ({ handleAction = () => {}, githubUrl = '#' }) => {
const ApprovedModal = ({ handleAction = () => {}, githubUrl = '#' }) => {
return (
<BasicModal
show={true}
centered={true}
closeButton={false}
content={
<>
<p className="thank-you-content">Your Review has been submitted!</p>
<p className="thank-you-share">Do you have anything else to share?</p>
<p className="thank-you-issue">
<p className="review-confirmation-content">
Your Review has been submitted!
</p>
<p className="review-confirmation-share">
Do you have anything else to share?
</p>
<p className="review-confirmation-issue">
<a href={githubUrl} target="_blank" rel="noreferrer">
Open a GitHub issue
</a>{' '}
to leave more feedback{' '}
</p>
</>
}
dialogClassName="thank-you"
dialogClassName="review-confirmation"
actions={[
{
label: 'Close',
onClick: handleAction
}
]}
title={
<div className="thank-you-title">
<div className="review-confirmation-title">
<FontAwesomeIcon
icon={faCheck}
className="thank-you-check"
className="review-confirmation-check"
color="green"
/>
<h1>Thank you!</h1>
Expand All @@ -45,9 +48,9 @@ const ThankYouModal = ({ handleAction = () => {}, githubUrl = '#' }) => {
);
};

ThankYouModal.propTypes = {
ApprovedModal.propTypes = {
handleAction: PropTypes.func,
githubUrl: PropTypes.string
};

export default ThankYouModal;
export default ApprovedModal;
51 changes: 51 additions & 0 deletions client/components/CandidateReview/CandidateModals/NotApprovedModal/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import React from 'react';
import BasicModal from '../../../common/BasicModal';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faCheck } from '@fortawesome/free-solid-svg-icons';
import PropTypes from 'prop-types';
import '../common.css';

const NotApprovedModal = ({ handleAction = () => {}, githubUrl = '#' }) => {
return (
<BasicModal
show={true}
centered={true}
closeButton={false}
content={
<>
<p className="review-confirmation-content">Thank you for reviewing</p>
<p className="review-confirmation-share">
if you haven’t opened any issues yet, please{' '}
<a href={githubUrl} target="_blank" rel="noreferrer">
open an issue describing why this is not approved.
</a>
</p>
</>
}
dialogClassName="review-confirmation"
actions={[
{
label: 'Close',
onClick: handleAction
}
]}
title={
<div className="review-confirmation-title">
<FontAwesomeIcon
icon={faCheck}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking an orange info circle or so could quickly reflect that this is the "non-final" path. I still thought I had incorrectly selected "Approved" instead of "Not Approved" at the end.

Just a thought, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The green check felt odd to me too but I wasn't confident enough about what sentiment we wanted to convey about 'not approved' reviews. Now that I know it feels odd to you as well, I'll swap it out.

Copy link
Contributor Author

@stalgiag stalgiag Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was challenging. I decided that the exclamation triangle felt most accurate after trying a few others including the info circle. That change is in this commit but I don't feel attached to that decision. That particular "i" just felt off for a modal that didn't have information but instead reflected the outcome of a decision. I felt that the exclamation mark conveyed the need for attention without being too severe since it is orange. Another option is a pause to indicate the process is on hold or waiting further approval.

Here are some screenshots. Do you have a preference?

modal with orange info i in circle modal with orange info i in circle
modal with orange triangle exclamation modal with orange triangle exclamation
modal with info i with no circle modal with info i with no circle

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not strongly tied to either (except the 'i' with no background). Moving forward with the triangle seems fine to me! But thanks so much for the thought you put into that

className="review-confirmation-check"
color="green"
/>
<h1>Thank you!</h1>
</div>
}
/>
);
};

NotApprovedModal.propTypes = {
handleAction: PropTypes.func,
githubUrl: PropTypes.string
};

export default NotApprovedModal;
1 change: 1 addition & 0 deletions client/components/CandidateReview/CandidateModals/ProvideFeedbackModal/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const ProvideFeedbackModal = ({
show={true}
centered={true}
cancelButton={false}
useOnHide={true}
handleHide={handleHide}
content={
<div className="feedback-content">
Expand Down
52 changes: 0 additions & 52 deletions client/components/CandidateReview/CandidateModals/ThankYouModal/ThankYouModal.css
View file
Open in desktop

This file was deleted.

53 changes: 53 additions & 0 deletions client/components/CandidateReview/CandidateModals/common.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,56 @@
.modal-header {
border-bottom: none;
}

.review-confirmation {
justify-content: center;
}

.review-confirmation .modal-content {
align-items: center;
width: 360px;
padding-top: 2em;
padding-bottom: 2em;
}

.review-confirmation-check {
font-size: 100px;
margin-right: 0 !important;
margin-bottom: 20px;
}

.review-confirmation-title {
text-align: center;
}

.review-confirmation-title h1 {
border-bottom: none;
font-size: 1.5em;
}

.review-confirmation-content {
font-weight: bold;
text-align: center;
}

.review-confirmation-share {
margin-bottom: 0;
}

.review-confirmation-issue {
text-align: center;
font-size: 12px;
margin-bottom: 0;
}

.review-confirmation .modal-header {
padding-bottom: 0;
}

.review-confirmation .modal-footer {
padding-bottom: 30px;
}

.review-confirmation .modal-footer .btn {
width: 100px;
}
42 changes: 24 additions & 18 deletions client/components/CandidateReview/CandidateTestPlanRun/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ import '../../App/App.css';
import { useMediaQuery } from 'react-responsive';
import TestPlanResultsTable from '../../common/TestPlanResultsTable';
import ProvideFeedbackModal from '../CandidateModals/ProvideFeedbackModal';
import ThankYouModal from '../CandidateModals/ThankYouModal';
import ApprovedModal from '../CandidateModals/ApprovedModal';
import FeedbackListItem from '../FeedbackListItem';
import DisclosureComponent from '../../common/DisclosureComponent';
import createIssueLink, {
getIssueSearchLink
} from '../../../utils/createIssueLink';
import RunHistory from '../../common/RunHistory';
import { useUrlTestIndex } from '../../../hooks/useUrlTestIndex';
import NotApprovedModal from '../CandidateModals/NotApprovedModal';

const CandidateTestPlanRun = () => {
const { atId, testPlanVersionId } = useParams();
Expand Down Expand Up @@ -63,7 +64,7 @@ const CandidateTestPlanRun = () => {
const [isFirstTest, setIsFirstTest] = useState(true);
const [isLastTest, setIsLastTest] = useState(false);
const [feedbackModalShowing, setFeedbackModalShowing] = useState(false);
const [thankYouModalShowing, setThankYouModalShowing] = useState(false);
const [confirmationModal, setConfirmationModal] = useState(null);
const [showInstructions, setShowInstructions] = useState(false);
const [showBrowserBools, setShowBrowserBools] = useState([]);
const [showRunHistory, setShowRunHistory] = useState(false);
Expand Down Expand Up @@ -151,9 +152,27 @@ const CandidateTestPlanRun = () => {
const submitApproval = async (status = '') => {
if (status === 'APPROVED') {
updateVendorStatus(true);
setConfirmationModal(
<ApprovedModal
handleAction={async () => {
setConfirmationModal(null);
navigate('/candidate-review');
}}
githubUrl={generalFeedbackUrl}
/>
);
} else {
setConfirmationModal(
<NotApprovedModal
handleAction={async () => {
setConfirmationModal(null);
navigate('/candidate-review');
}}
githubUrl={generalFeedbackUrl}
/>
);
}
setFeedbackModalShowing(false);
setThankYouModalShowing(true);
};

useEffect(() => {
Expand Down Expand Up @@ -631,7 +650,7 @@ const CandidateTestPlanRun = () => {
</Row>
</Col>
</Row>
{feedbackModalShowing ? (
{feedbackModalShowing && (
<ProvideFeedbackModal
at={at}
show={true}
Expand All @@ -654,21 +673,8 @@ const CandidateTestPlanRun = () => {
handleAction={submitApproval}
handleHide={() => setFeedbackModalShowing(false)}
/>
) : (
<></>
)}
{thankYouModalShowing ? (
<ThankYouModal
show={true}
handleAction={async () => {
setThankYouModalShowing(false);
navigate('/candidate-review');
}}
githubUrl={generalFeedbackUrl}
/>
) : (
<></>
)}
{confirmationModal && confirmationModal}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically unnecessary but I thought it helped convey the possible of a no render for the modal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, thinking you don't have to make conveying this clear but {!!confirmationModal && confirmationModal} could also display that?

</Container>
);
};
Expand Down
14 changes: 0 additions & 14 deletions client/stories/ThankYouModal.stories.jsx
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted this while renaming because it was just a stub without meaningful customization and we aren't using stories as far as I understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems fine.

Part of me has felt keeping the storybook setup to be unnecessary. Historically, it hasn't gotten much traffic during development time and that's also reflected currently with only 2 storybook components being defined. Some only being created after the fact, so lacking for prototyping and certainly not much created after component creation so not much done in the way of functionalliy documenting components' usages. I'm all for the concept but not when there isn't any strong support for it -- right now at least.

Another thing is this project's versions are severely outdated and should probably get some updates!

This file was deleted.

Loading