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

[WIP] Regulation Page - Hardcoded data #365

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Kameron-Heyen
Copy link
Collaborator

No description provided.

Copy link
Contributor

@zlannin zlannin left a comment

Choose a reason for hiding this comment

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

I think we should make sure we use Regulatory instead of Regulation. Regulation implies an actual rule or process whereas regulatory describes anything involved in making, enforcing, or related to regulations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote to rename this accessor to regulatoryAccessor.ts

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kameron-Heyen @zlannin we need to drop the word "regulatory", actually. The use of "regulatory" came from a bit of a misunderstanding. We're getting information on an overylay, which could be regulatory, administrative, etc.

import axios from "axios";

// TODO: Wire to API when route is created
export const getRegulationDetails = async (areaUuid: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const getRegulationDetails = async (areaUuid: string) => {
export const getRegulatoryDetails = async (areaUuid: string) => {

.water-rights-card, .site-card {

.water-rights-card, .site-card, .regulation-card {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.water-rights-card, .site-card, .regulation-card {
.water-rights-card, .site-card, .regulatory-card {

Copy link
Collaborator

@tarrball tarrball Nov 18, 2024

Choose a reason for hiding this comment

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

any reason why we're not just using a .detail-card class, instead of giving the same class three (soon four) aliases?

Copy link
Contributor

Choose a reason for hiding this comment

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

update parent folder from regulation to regulatory


type ActiveTabType = "right" | "regulatory";

interface RegulationDetailsPageContextState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface RegulationDetailsPageContextState {
interface RegulatoryDetailsPageContextState {

Copy link
Contributor

Choose a reason for hiding this comment

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

useRegulatoryQuery.ts

import { UseQueryOptionsParameter } from "../../HelperTypes";
import { RegulatoryOverlayInfoListItem } from "@data-contracts";

export function useRegulationDetails(areaUuid: string | undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function useRegulationDetails(areaUuid: string | undefined) {
export function useRegulatoryDetails(areaUuid: string | undefined) {


export function useRegulationDetails(areaUuid: string | undefined) {
return useQuery(
["regulation.Details", areaUuid],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
["regulation.Details", areaUuid],
["regulatory.Details", areaUuid],

Copy link
Contributor

Choose a reason for hiding this comment

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

RegulatoryDetailsPage.tsx

import { Layout } from '../components/details-page/regulation/Layout';
import {RegulationDetailsProvider} from "../components/details-page/regulation/Provider";

function RegulationDetailsPage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function RegulationDetailsPage() {
function RegulatoryDetailsPage() {

@@ -74,6 +75,7 @@ function App({ msalInstance }: AppProps) {
<Route path="details" element={<DetailLayout />}>
<Route path="site/:id" element={<SiteDetailsPage />} />
<Route path="right/:id" element={<WaterRightDetailsPage />} />
<Route path="regulation/:id" element={<RegulationDetailsPage />} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Route path="regulation/:id" element={<RegulationDetailsPage />} />
<Route path="overlay/:id" element={<OverlayDetailsPage />} />

WaterRightInfoListItem,
} from "@data-contracts";

type Query<T> = Pick<
Copy link
Collaborator

Choose a reason for hiding this comment

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

opportunity for a shared type?

useSiteDigestMapPopup();
useMapLegend();

if (locationsQuery.isLoading || !locationsQuery.data) return null;
Copy link
Collaborator

@tarrball tarrball Nov 18, 2024

Choose a reason for hiding this comment

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

IMO you should always avoid these types of one liners. It's tougher to see that this is a function exit/jump (a return, a throw, a break, a continue) at a glance, and it's harder to get a breakpoint in there if you ever want to step through.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (locationsQuery.isLoading || !locationsQuery.data) return null;
if (locationsQuery.isLoading || !locationsQuery.data) {
return null;
}

return beneficialUses.map((use) =>
use !== beneficialUses[beneficialUses.length - 1] ? `${use}, ` : use,
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's figure out a way to share functions like these if they're just gonna be copy/pasted all over.

Having said that, is this the same as:

beneficialUses.join(', ');

?

@@ -0,0 +1,32 @@
/*
This file is largely duped from other existing maps in site and water rights pages. Potential opportunity to create a more reusable component for this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's super hard to look at this PR and give good feedback with so much stuff copy/pasted over at once. I can't tell what is something you put thought into (and should be scrutinized) and what is some stuff that you didn't write, but just copy/pasted. The backlog we started from wasn't the best, but we'll have a better one moving forward. Should be easier to task things out into reasonable chunks.

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.

3 participants