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

Jump to bottom

Improve appearance of inspection section #1016

Merged
merged 14 commits into from
Nov 1, 2023
Merged

Improve appearance of inspection section #1016

merged 14 commits into from
Nov 1, 2023

Conversation

mrica-equinor
Copy link
Contributor

@mrica-equinor mrica-equinor commented Sep 20, 2023 •
edited
Loading

Closes #1021

@mrica-equinor mrica-equinor self-assigned this Sep 20, 2023
@mrica-equinor mrica-equinor added the frontend Frontend related functionality label Sep 20, 2023
@mrica-equinor mrica-equinor added the backend Backend related functionality label Sep 21, 2023
@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch 4 times, most recently from 3c14a12 to a792ec2 Compare September 22, 2023 11:46
@mrica-equinor mrica-equinor marked this pull request as ready for review September 22, 2023 11:52
@OleDrange
Copy link
Contributor

OleDrange commented Sep 25, 2023 •
edited
Loading

AP300 example
In Figma, the cards have the status text on them. See image. Can you implement this as well? I see that we have 1 circle that says Missions, but it looks better with up to 3 circles and status text after it.

@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch from a792ec2 to 620082c Compare October 4, 2023 15:26
@mrica-equinor
Copy link
Contributor Author

AP300 example In Figma, the cards have the status text on them. See image. Can you implement this as well? I see that we have 1 circle that says Missions, but it looks better with up to 3 circles and status text after it.

This has been addressed

andchiind
andchiind reviewed Oct 4, 2023
View reviewed changes
Copy link
Contributor

@andchiind andchiind left a comment

Choose a reason for hiding this comment

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

Looks good overall, I'm just nitpicking

frontend/src/components/Pages/FrontPage/FrontPage.tsx Outdated Show resolved Hide resolved
frontend/src/components/Pages/InspectionPage/InspectionOverview.tsx Outdated Show resolved Hide resolved
frontend/src/components/Pages/InspectionPage/InspectionSection.tsx Outdated Show resolved Hide resolved
frontend/src/components/Pages/InspectionPage/InspectionTable.tsx Outdated Show resolved Hide resolved
frontend/src/language/no.json Outdated Show resolved Hide resolved
@Eddasol
Copy link
Contributor

Eddasol commented Oct 4, 2023

image
Doesn't look great when we have missions with all 3 statuses

@Eddasol
Copy link
Contributor

Eddasol commented Oct 4, 2023

image
"Never runs" are red when seeing separate missions but green on the deck summary

@Eddasol
Copy link
Contributor

Eddasol commented Oct 4, 2023

image
On missions that have never been run I suggest setting "overdue" or something similar under deadline instead of leaving it empty

@Eddasol
Copy link
Contributor

Eddasol commented Oct 4, 2023 •
edited
Loading

image

The mission with no inspection frequency doesn't have a circle. Suggest either gray or green

@Eddasol
Copy link
Contributor

Eddasol commented Oct 4, 2023

image
Capital letter on all words in "Deck Overview" but only capital letter on first word in "Predefined missions"

@mrica-equinor
Copy link
Contributor Author

image "Never runs" are red when seeing separate missions but green on the deck summary

image

It doesn't have a deadline so the question here is do we want to mark it as red or green? I thought for it to be green because there's no deadline but can also agree that it could be red because it has never been performed. But do we want to perform it and therefore mark it red if no deadline is in place?

@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch 3 times, most recently from fd2d79b to 958a3e6 Compare October 5, 2023 14:36
@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch from 35499ab to 1cab3c3 Compare October 6, 2023 13:38
Eddasol
Eddasol reviewed Oct 30, 2023
View reviewed changes
frontend/src/components/Pages/InspectionPage/InspectionSection.tsx
Comment on lines +193 to +83
useEffect(() => {
if (selectedMissions) {
let unscheduledMissions: CondensedMissionDefinition[] = []
selectedMissions.forEach((mission) => {
if (Object.keys(scheduledMissions).includes(mission.id) && scheduledMissions[mission.id])
setIsAlreadyScheduled(true)
else unscheduledMissions = unscheduledMissions.concat([mission])
})
setUnscheduledMissions(unscheduledMissions)
}
}, [isDialogOpen])
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduces warning
image

If you only want to trigger this behavior on dialogOpen, would it make more sense to have a function that is called when you open the dialog instead of an useEffect?

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'm not sure why but changing this to a function breaks the behavior, i know when I wrote the code I knew why, but will need a moment to understand it again

Eddasol
Eddasol reviewed Oct 30, 2023
View reviewed changes
frontend/src/components/Pages/InspectionPage/InspectionTable.tsx
Comment on lines +326 to +337
useEffect(() => {
let unscheduledMissions: CondensedMissionDefinition[] = []
if (selectedMissions) {
selectedMissions.forEach((mission) => {
if (Object.keys(scheduledMissions).includes(mission.id) && scheduledMissions[mission.id])
setIsAlreadyScheduled(true)
else unscheduledMissions = unscheduledMissions.concat([mission])
})
setUnscheduledMissions(unscheduledMissions)
}
}, [isDialogOpen])
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduces warning
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this warning on several places and I'm honestly not sure what the fix here is

Copy link
Contributor

Choose a reason for hiding this comment

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

We can fix it in another PR imo

@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch 3 times, most recently from 7ac9624 to 0260a78 Compare October 30, 2023 15:43
andchiind
andchiind approved these changes Nov 1, 2023
View reviewed changes
Copy link
Contributor

@andchiind andchiind left a comment

Choose a reason for hiding this comment

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

LGTM, small issues like warnings can be fixed in another PR imo as this one is getting big

@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch from 0260a78 to eaea166 Compare November 1, 2023 09:52
@mrica-equinor mrica-equinor force-pushed the inspection-page-design branch from eaea166 to 26cddd0 Compare November 1, 2023 09:56
@mrica-equinor mrica-equinor merged commit 59462a5 into equinor:main Nov 1, 2023
7 checks passed
@mrica-equinor mrica-equinor deleted the inspection-page-design branch May 13, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eddasol Eddasol Eddasol left review comments

@andchiind andchiind andchiind approved these changes

Assignees

@mrica-equinor mrica-equinor

Labels
backend Backend related functionality frontend Frontend related functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve appearance of inspection page
5 participants
@mrica-equinor @OleDrange @Eddasol @prasm313 @andchiind