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

Include Drafts in Recently Viewed Documents #38

Merged
merged 18 commits into from
Mar 23, 2023

Conversation

jeffdaley
Copy link
Contributor

@jeffdaley jeffdaley commented Feb 9, 2023

Moves RecentlyViewedDocument functions from the dashboard route to a service. By detaching from the model hook, we make fewer network calls and get a faster, more reactive dashboard.

The service also expands to include drafts, and to accommodate legacy users, we transform the old list:

files: ['docID1', 'docID2', 'docID3']

into the new format when fetchAll is called by the dashboard route:

files:[
    { id: 'docID1', isDraft: false },
    { id: 'docID2', isDraft: false },
    { id: 'docID3', isDraft: false }
]

You can see for yourself locally. Log in and your recently viewed docs should be preserved. (Note: Your recent-docs list will reset when switching back to main.)

@jeffdaley jeffdaley marked this pull request as ready for review February 9, 2023 20:47
@jeffdaley jeffdaley requested a review from a team as a code owner February 10, 2023 22:09
Copy link
Contributor

@jfreda jfreda left a comment

Choose a reason for hiding this comment

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

Overall looks really good! Just wondering about error checking in a couple of spots so the dashboard doesn't break in case these new service methods error out.

return recentlyViewedDocs;
});
if (this.recentDocs.all === null) {
await this.recentDocs.fetchAll.perform();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add some error checking here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What did you have in mind? Here I'm relying on fetchAll's error handling

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like fetchAll just logs and throws an error so it would end up here? I might be missing something though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this isn't necessarily a blocker, but I just wanted to make sure that the dashboard would still load if there was an error with fetchAll - did you test that?

Copy link
Contributor Author

@jeffdaley jeffdaley Mar 23, 2023

Choose a reason for hiding this comment

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

I see what you mean. Check out these changes: a634c7d

We now let failed fetchAlls through and show an error in the template with the option to refresh.

In this example it's hard-coded to fail, but it should give you an idea of the implementation:

CleanShot.2023-03-22.at.21.00.57.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great! :shipit:

web/app/routes/authenticated/document.js Show resolved Hide resolved
Copy link
Contributor

@jfreda jfreda left a comment

Choose a reason for hiding this comment

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

Nice, just one sorta open question about error handling, but otherwise LGTM!

@jeffdaley jeffdaley merged commit 6d736c7 into main Mar 23, 2023
@jeffdaley jeffdaley deleted the jeffdaley/recent-docs-and-drafts branch March 23, 2023 16:49
anuragprafulla referenced this pull request in razorpay/hermes Jun 27, 2023
* Add drafts to recentlyViewedDocs

* Partial Mirage responses

* Additional Mirage responses; initial passing test

* Additional tests and template fixes

* Include `@isDraft` arg

* Write LegacyUsers test

* Fix config and broken tests

* Cleanup

* Fix type errors; Update Mirage responses

* Handle RecentDocs error in dashboard
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.

None yet

2 participants