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

Update Entry Submit to point at existing endpoint #48 #49

Closed
wants to merge 5 commits into from

Conversation

syedfkabir
Copy link
Contributor

Resolves #48

Changes

  • Changed the PersonWeeklyEntries in /pages/people/person/Timesheet.tsx to now follow 'entry-collection'
  • Changed the EntryDayItemNew component to have the parent component postFollow instead of post

Notes for PR

  • If you try to submit a Entry without first selecting a Project, you will get a console error and the POST will not go through. There needs to be a follow up ticket for this
  • When you decrease the size of the screen, you can no longer see the selected project

Screenshots

Submitting without a selected project

Screen Shot 2022-10-20 at 12 53 54 PM

Minimized screen showing no selected project (even though there is)

Screen Shot 2022-10-20 at 12 54 12 PM

@@ -48,7 +48,7 @@ export function PersonWeeklyEntries(props: Props) {
{[0, 1, 2, 3, 4, 5, 6].map( val =>
<EntryDay
date={currentDate.plus({'days': val})}
resource={resourceState.follow('search-sheet', { year: currentDate.year, weekNum: currentDate.weekNumber })}
resource={resourceState.follow('entry-collection')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a hunch this change means that entries are no longer filtered by weeks, so every week shows all entries in the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made some changes to address this!
tested and ready to review

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify this more, we must continue to use the existing search-sheet resource for this because if we don't, you are loading in every time entry ever made.

We need to continue to filter by week. The search-sheet link was added so we can do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So even though visually this might do the right thing. If a user has thousands of entries, this change would load all of them in and we don't want that.

Copy link
Contributor

@BeckyPollard BeckyPollard left a comment

Choose a reason for hiding this comment

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

Might be in the same spirit as Evert's feedback, but on testing this, seems we've lost the time entries rendering. After submit the entry just vanishes. DB updates though which is rad.

Visual example
Above: current state as of review.
Below: main branch with time entries rendering.


@syedfkabir
Copy link
Contributor Author

PR Closed due to: #49 (comment)
Continuing here: badgateway/tt-api#36

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.

Update Entry Submit to point at existing endpoint
3 participants