Skip to content

Conversation

@stopfstedt
Copy link
Member

@stopfstedt stopfstedt commented Nov 19, 2025

this doesn't do anything meaningful and gets in the way of using a more robust data structure, like a class with getters or a Proxy object. begone!

extracted from #8956

side-note

further down, we're modifying the events by setting a isMulti flag if applicable. i believe that the removed code is related to that, although i can't back that claim up based on commit messages.

ideally, we'd not be modifying these events at all, once they're generated. conceptually, they are "read only".

will explore a solution that gets rid of a event.isMulti as a separate issue.

@netlify
Copy link

netlify bot commented Nov 19, 2025

Deploy Preview for ilios-frontend ready!

Name Link
🔨 Latest commit 4db5d76
🔍 Latest deploy log https://app.netlify.com/projects/ilios-frontend/deploys/691e1a2d3f704200082c5589
😎 Deploy Preview https://deploy-preview-8960--ilios-frontend.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

this doesn't do anything meaningful and gets in the way of using a more
robust data structure, like a class with getters or a Proxy object.
begone!
@stopfstedt stopfstedt force-pushed the seven_seven_with_trample branch from e781862 to 4db5d76 Compare November 19, 2025 19:27
@stopfstedt stopfstedt marked this pull request as ready for review November 19, 2025 19:32
@jrjohnson
Copy link
Member

jrjohnson commented Nov 19, 2025

My only concern is the //clone our event, so we don't trample on the original when we change location seems to point to this doing something very specific. I don't know what though. I'm guessing it's the isMulti where we change the location to tbd? I don't have any memory for how these fit together.

@stopfstedt
Copy link
Member Author

My only concern is the //clone our event, so we don't trample on the original when we change location seems to point to this doing something very specific. I don't know what though. I'm guessing it's the isMulti where we change the location to tbd? I don't have any memory for how these fit together.

this has to do with displaying multiple events as one in the weekly and monthly calendar. if it's a multi-event, clicking on it will take you to the calendar's day-view with all similar events expanded.
non-multi event will let you click through to the event's detail screen directly.

@jrjohnson jrjohnson removed the request for review from michaelchadwick November 19, 2025 23:24
@jrjohnson jrjohnson added the run ui tests Run the expensive UI tests label Nov 19, 2025
@stopfstedt
Copy link
Member Author

@dartajax - for testing purposes - this change affects the dashboard calendar only.

regression testing should focus on events on the week- and month-view that expand to multiples in the day-view when clicked upon.

no functional changes are expected, this should work as before.

Copy link
Member

@dartajax dartajax left a comment

Choose a reason for hiding this comment

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

click-tested per instructions from @stopfstedt - seems fine - no changes noticed on Dashboard calendar - clicking into multiples from Week or Month view - behaves as before so ... approval granted

@dartajax dartajax merged commit d37f63c into ilios:master Nov 20, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run ui tests Run the expensive UI tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants