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

Add Reef check survey page #1060

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

K-Markopoulos
Copy link
Collaborator

@K-Markopoulos K-Markopoulos commented Nov 22, 2024

Note: requires backend changes from #1052 to work.

Add reef check survey page in UI. Designs

Next steps for this PR:

  • Add tests
  • Adjust for mobile view
  • Add skeletons to improve loading experience, handle errors
  • Reconsider details layout (from flex to table) We keep the flex with media queries for column-count
  • Add missing tables
  • Tweak styles to be closer to designs
  • Add Request to download functionality (we will likely add it in another PR)

image

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

⚠️ This pull request was not sent to the PullRequest network because the pull request is a draft.

Copy link

github-actions bot commented Nov 29, 2024

Build succeeded and deployed at https://aqualink-app-1060.surge.sh
(hash dec9873 deployed at 2024-12-03T17:01:25)

@K-Markopoulos K-Markopoulos marked this pull request as ready for review November 29, 2024 12:58
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:
  • Comments posted to any areas of potential concern or improvement.
  • Detailed feedback or actions needed to resolve issues that are found.
  • Turnaround times vary, but we aim to be swift.

@K-Markopoulos you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 1,476
- 1

44% TSX
39% TypeScript
17% TSX (tests)
<1% JSON
<1% Other

Type of change

Feature - These changes are adding a new feature or improvement to existing code.
1 Message
⚠️ Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

I've given this MR a quick first pass, and noticed an issue, noted inline. Second pass to follow, shortly.

Image of Graham C Graham C


Reviewed with ❤️ by PullRequest


export const getReefCheckSurvey = async (siteId: string, id: string) =>
requests.send<ReefCheckSurvey>({
url: `reef-check-sites/${siteId}/surveys/${id}`,
Copy link

Choose a reason for hiding this comment

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

It's necessary to URI-encode the ids here to prevent URL confusion attacks.

url: `reef-check-sites/${encodeURIComponent(siteId)}/surveys/${encodeURIComponent(id)}`,

🔺 Vulnerability (Error)

Image of Graham C Graham C

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch, done.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

I've completed my second pass, and noticed just one additional issue, besides the smattering of TODO comments.

Image of Graham C Graham C


Reviewed with ❤️ by PullRequest

a 100-meter transect line for a total survey area of 400m². Each
segment has an area of 100m² and is labelled as s1, s2, s3, or s4.
</Typography>
<Link
Copy link

Choose a reason for hiding this comment

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

Does the link component add `target="noopener noreferrer" for you? If not, I'd strongly recommend adding it.

🔺 Vulnerability (Error)

Image of Graham C Graham C

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't, but as I found out all major browsers imply rel='noreferrer' on target='_blank' links. I added it anyway, thanks for the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants