belindas-closet-nextjs_10_526_Add_charts #527
Conversation
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
Alzavio
changed the title
|
@Alzavio Great job. can you resolve conflicts and post a screenrecording of the feature? Thanks |
There was a problem hiding this comment.
I made some changes on my end in order for this to show:
I ran npm install chart.js react-chartjs-2 , then npm run build npm run dev to get it started.
I created a "weeklyActivity" folder and placed it within the "dashboard" folder for the "weeklyActivity.tsx" file to live. I did also rename the "weeklyActivity.tsx" to "page.tsx" due to funkiness that React can exhibit when things are named differently.
I changed the import for WeeklyActivity on the dashboard page.tsx to import WeeklyActivity from "@/app/dashboard/weeklyActivity/page"; due to changing the location of the file in reference.
Based upon that, perhaps implementing those changes can help the errors?
There was a problem hiding this comment.
This is what I see on my end:
Looks good, seems like the changes @nwm516 suggested might have helped with the errors you were having. also, data is highlighted when I hover over the bar itself so good work on that.
I am noticing minor bugs and funkiness with the chart such as this image below:
there is also endless navigation with the arrows and the data isn't consistent but since this is just dummy data and getting a basic chart going its ok for now @Alzavio.
sidenote: the bar graphs on the bottom for 30 days is thinner than the one shown in @nwm516 but it's a minor change if we need to change the size of them.
Looks like you still need to resolve the branch conflicts still, but ill approve for now because the chart is showing up for me.
Changed files to deal with conflicts that had arisen regarding the weeklyActivity section of Belinda's Closet dashboard.
nwm516
force-pushed
the
feature-526-Add-charts
branch
from
a351bf9 to
1ffb29a
Compare
…oset-nextjs into backup-feature-526-Add-charts
Updated package-lock.json and ran "npm ci"
Update page.tsx for clashing properties with dashboard
nwm516
requested review from
bcko,
keiffer213,
BradleyCharles,
gitbiruk2010,
JesseCaddell,
taylorpapke and
miqo1385
and removed request for
theGaryLarson and
heosman
There was a problem hiding this comment.
This is what I see on my end as per the update @nwm516, I ran npm ci as you did yourself.
The sidebar menu does cover part of the charts, but I suppose that can be fixed later, since this is just implementing the chart with dummy data, its fine how it looks for now.
I also did notice these console errors when running npm test
the console errors are fine for now at this stage, but they should be corrected at some point, I think.
Other than that, the only small thing I would request is the removal of carets in the package.json file (should've mentioned it before in earlier post) and this PR should be good for approval. Thanks for the work! @nwm516.
There was a problem hiding this comment.
Findings:
run 'npm test' and gives the same console error as asfand00 mentioned.
sidebar menu remains the same
running 'npm ci': since it is performing a clean install, which is strictly as listed in package-lock.json, of course it messes with the new sample data and existing page in app/dashboard/ & app/dashboard/weeklyActivity/ page.
Otherwise, the rest looks good on my end.
dashboard-chart-test.mp4
I think we will get to the rest of the fixes once we have a real data.
Resolves 526
Progress Summary: Adds dummy charts in the style of the Figma design until we have data
Key Learnings: N/a, I'm used to Chart.js