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

UI - Input layout #231

Merged
merged 23 commits into from
Jan 9, 2025
Merged

UI - Input layout #231

merged 23 commits into from
Jan 9, 2025

Conversation

gabrielegranello
Copy link
Collaborator

@gabrielegranello gabrielegranello commented Jan 7, 2025

What does this PR do?

This is a work in progress on the UI of the calculator. It sets the font style and the initial grid.
It also adds a further component called FormEdit.tsx in the dashboard, so that the user knows which input data have been used for the analysis.

Webflow to match here https://fairhold.webflow.io/classbook

Copy link

vercel bot commented Jan 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fairhold-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 2:37pm

@@ -25,6 +25,7 @@ import { Label } from "@/components/ui/label";
const CalculatorInput = () => {
const [view, setView] = useState("form");
const [data, setData] = useState<Household | null>(null);
const [formData, setFormData] = useState<formType | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to hold these values in state a second time - we can already access this with methods.getValues()

I'm not sure if methods is a standard naming convention - maybe we just call it form?

return (
<Dashboard
processedData={data as Household}
inputData={formData as formType}
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment - I think the below should work.

Suggested change
inputData={formData as formType}
inputData={methods.getValues()}

app/globals.css Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't appreciated how much of this was redundant and just webflow styles (w-*).

Maybe it would be simpler to not import this at all and just emulate / copy styles ad-hoc. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually - here's a better idea 💡

If we just make a file called webflow.css which is just copy/pasted from the source, and then we can use import "./webflow.css" in layout.tsx

This should give us easy access to the styles, without cluttering our styles?

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Looking good! If you're please able to resolve the merge conflicts I'll review this shortly 👌

@gabrielegranello
Copy link
Collaborator Author

Sorry almost good to go, keep failing the coverage test. I am going to try a couple of other times, otherwise I night just add a couple of extra lines in the test to increase the coverage

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Visually great 👍

A few small comments and suggestions on the code.

app/components/ui/Dashboard.tsx Outdated Show resolved Hide resolved
app/components/ui/Dashboard.tsx Outdated Show resolved Hide resolved
app/components/ui/FormEdit.tsx Outdated Show resolved Hide resolved
app/components/ui/Dashboard.tsx Show resolved Hide resolved
app/components/ui/CalculatorInput.tsx Outdated Show resolved Hide resolved
Comment on lines 340 to 346
} else if (view === "dashboard") {
return (
<Dashboard
processedData={data as Household}
inputData={form.getValues()}
/>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the changes made in #243 have been lost when dealing with the merge conflicts here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @DafyddLlyr I should have picked up what was lost when merging

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

One small comment on code 👍

Also the inputs' text colours need to be black / darker than the placeholder colour -

image

Comment on lines 339 to 346
} else if (view === "dashboard" && data) {
const formValues = form.getValues();
return (
<Dashboard
processedData={data}
inputData={formSchema.parse(formValues)}
/>
);
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
} else if (view === "dashboard" && data) {
const formValues = form.getValues();
return (
<Dashboard
processedData={data}
inputData={formSchema.parse(formValues)}
/>
);
}
if (view === "dashboard" && data) {
const formValues = form.getValues();
return (
<Dashboard
processedData={data}
inputData={formValues}
/>
);

At this point, the values are already parsed so we don't need to repeat this 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully fixed thanks @DafyddLlyr

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Great!

@gabrielegranello gabrielegranello merged commit 414e75f into main Jan 9, 2025
5 checks passed
@gabrielegranello gabrielegranello deleted the gg/ui-input-layout branch January 9, 2025 14:53
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