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

Join session milestone point #43

Closed
wants to merge 31 commits into from
Closed

Conversation

qafui
Copy link
Collaborator

@qafui qafui commented Oct 12, 2024

Description

This PR adds the useSWR hook from the swr library as a wrapper around a global fetcher hook (useApiData). It allows any component to access the data it needs for rendering dynamic values from the API.

A dashboard component is refactored to use the new hook to fetch data for conditionally rendered select components and a button. The JoinCoachingSession component allows a user to select an organization, a coaching relationship and a session and join the session with a button click.

Todo

  • Separate previous from upcoming sessions with a SelectGroup
  • Merge and fix conflicts

Changes

  • Adds a hook for fetching data from the API
  • Adds a Dynamic select component that renders dropdowns with data fetched from the API
  • Renders 3 dynamic select components in a composite or ‘smart’ JoinCoachingSession component responsible for setting organization, relationship and session IDs in the state tree.

Screenshots / Videos Showing UI Changes (if applicable)

Screenshot 2024-10-03 at 3 12 08 PM

Testing Strategy

Login and attempt to join a session using the component illustrated in the image

Concerns

  • The rendering helper functions in the dynamic-api-select component should be separated into its own component.
  • PRs of this kind should be broken up into smaller features in future to improve code merge 😅

@qafui qafui marked this pull request as ready for review October 12, 2024 04:16
@jhodapp jhodapp added the enhancement Improves existing functionality or feature label Oct 16, 2024
Copy link
Member

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

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

Thanks David, it's looking great and very close. I left a few changes inline to take a look at.

@@ -148,7 +148,6 @@ const OverarchingGoalContainer: React.FC<{
);
});
}
fetchOverarchingGoal();
Copy link
Member

Choose a reason for hiding this comment

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

@qafui this is necessary, right now if you go to a coaching session page, if you've set an OverarchingGoal, it isn't getting loaded when the page loads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! I should have re-tested that bit. I noticed it was re-rendering in the console and thought to fix it with memoization but the thought somehow trailed off. I was too tired 😸

@@ -46,26 +48,25 @@ export default function CoachingSessionsPage() {
const [note, setNote] = useState<Note>(defaultNote());
const [syncStatus, setSyncStatus] = useState<string>("");
const { userId } = useAuthStore((state) => ({ userId: state.userId }));
const { coachingSession } = useAppStateStore((state) => state);
const coachingSessionId = useAppStateStore(
Copy link
Member

Choose a reason for hiding this comment

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

This is ok if it fixes the fetch loop (not sure if it was the cause or not). We'll convert to de-duplicating storing these Ids separately from their object instances after we land this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not the cause but a small optimization after observing that we weren't using any other properties of the object in the component.

I could be wrong but the way I understand it, this is a case of whether we want to pass around the string value or the reference to the object (while operating on its mutable property).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm open to your thoughts here on what's best long term for convenience and efficiency.

src/components/ui/dashboard/join-coaching-session.tsx Outdated Show resolved Hide resolved
url={`/organizations/${organizationId}/coaching_relationships`}
params={{ organizationId }}
onChange={handleRelationshipSelection}
placeholder="Select coaching relationship"
Copy link
Member

Choose a reason for hiding this comment

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

Same here (see above comment about organizationId).

src/components/ui/dashboard/join-coaching-session.tsx Outdated Show resolved Hide resolved
src/components/ui/dashboard/join-coaching-session.tsx Outdated Show resolved Hide resolved
<div className="grid gap-2">
<Label htmlFor="session-selector">Coaching Session</Label>

<DynamicApiSelect<CoachingSession>
Copy link
Member

Choose a reason for hiding this comment

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

For some reason I'm seeing duplicate past sessions and 1-2 of them are out of chronological order as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see that in the db as well. Not sure how that got added tbh.

Copy link
Member

Choose a reason for hiding this comment

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

Oh you see it duplicated in the DB itself? In that case it's an error seeding data on the backend. I'll try changing it locally and see if this issue goes away.

Copy link
Member

Choose a reason for hiding this comment

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

I found the culprit in the backend and fixed.

Copy link
Member

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

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

Looking great, just a couple more issues to address and I think it might be ready to approve (I'll give it one last look over again).

src/app/coaching-sessions/[id]/page.tsx Outdated Show resolved Hide resolved
to_Date: TO_DATE,
}}
onChange={handleSessionSelection}
placeholder={sessionPlaceholder}
Copy link
Member

Choose a reason for hiding this comment

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

When you select a session and join, and then return to the dashboard page, it'll show the date of the selected session but it's in a raw datetimestampz format instead of the same format used in the drop down list in the selector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup! See slack DM! That was the last bit driving me mad 😠. Not an ideal solution but I'll add a reformatter to the option content itself. Let me know if you can think of something else in the sorting part of the dynamic select component.

Copy link
Member

Choose a reason for hiding this comment

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

The sorting looks good, it's just the placeholder text. So if you add a formatter making sure the date renders correctly, that should fix the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turned out to be value of the date in the state tree. 😄

Co-authored-by: Jim Hodapp <james.hodapp@gmail.com>
@jhodapp
Copy link
Member

jhodapp commented Oct 31, 2024

There seems to be another bug where the coaching session select displays coaching sessions that don't apply to the coaching relationship.

So if I select "Refactor Coaching" -> "Jim Hodapp -> Caleb Bourg" I do expect to see a list of coaching sessions (these are the only sessions seeded by default and only for this relationship).

But if I select "Acme Corp" -> "Jim Hodapp -> Other Users" I would expect no coaching sessions to be found. Here's a screenshot of the invalid behavior:

Screenshot 2024-10-31 at 11 09 09

@jhodapp
Copy link
Member

jhodapp commented Oct 31, 2024

@qafui I do see a correctly formatted date on the dashboard page now, but unfortunately I see a regression now on the coaching session page of an invalid date:

Screenshot 2024-10-31 at 12 02 46

It's because you change the underlying date string format in the stored object. If you leave that alone and just render it correctly here, the coaching session page should be ok then.

@qafui
Copy link
Collaborator Author

qafui commented Oct 31, 2024

Thanks for catching those. Will let you know when they're resolved.

@qafui qafui marked this pull request as draft November 12, 2024 22:23
@qafui qafui closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves existing functionality or feature
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants