-
Notifications
You must be signed in to change notification settings - Fork 7
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
React cal poc kl #85
React cal poc kl #85
Conversation
…yedEvents, and allow multiple event components to be displayed for a selected date
…ight to min-content
✅ Deploy Preview for dev-fcccolumbus ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@KVinyl please resolve the merge conflicts and the linting errors |
@readysetagile Merge conflicts and linting errors have been resolved. Did some further refactoring. Of course, more needs to be done, just looking for feedback for I have so far. |
@KVinyl a test is failing. Make sure you run |
@readysetagile Updated the snapshots to pass testing. |
7b83732
to
dd2e133
Compare
…ventDateTimeSet to help optimize event date search
@readysetagile Further optimization, refactoring, minor bug fixes |
…layedEvents to derive from eventsByDateString
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it works great, so nice work there!
I think that we might want to follow good design patterns here also -- like a MVC. So let's move the code that is currently in Events.jsx
into a controller structure, that is, eventsController.js
The model is already taken care of in the calendar control. That way Events.jsx
becomes the "view", and just handles user events and initialization. you can ask GPT this question to know what I mean:
can this react.js code for Events.jsx be refactored so that it follows a view-controller design, that is, the Events.jsx serves as a view to just handle user events and initialization, and the controller does the stuff that happens when an event is fired? [include the code for Events.jsx]
Seeing this will also likely bring up more refactoring, like being able to configure the link for the calendar through a file and possibly make it data driven also -- along with other maintainability concerns, so let's start with the controller.
@readysetagile Created two custom hooks. useEvents is responsible for fetching events. useCalendar takes in events as an input and returns the variables/functions used by the Calendar/Events section. Fixed unwanted multiple events CSS behavior in Chrome. |
const fetchData = async () => { | ||
try { | ||
const response = await fetch( | ||
'https://docs.google.com/document/d/1OcnWWt1qHaJWE-8v_FPtNO8DKviRQ7DMaqylupuvPXE/export?format=txt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will create another backlog item to make this value configurable. It's outside the scope of this backlog item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done, @KVinyl ! This captures the essence of a good design.
Changelog from #79
Moved calendar and event cards to one row.
Allow multiple event cards to be displayed.
Scroll bar is shown when multiple event cards are displayed.
No event cards are shown when selected date has no events.
Text color of day number during the weekend in calendar is white when selected or has event instead of black.
--
Event card text needs formatting.
Does not look good on mobile.
Lots of module warnings during webpack compilation.