refactor: elevate download with donation to component level #3869
Conversation
mchen10
force-pushed
the
3858-elevate-download-with-donation
branch
from
dfca33c
to
7922e02
Compare
|
Figured it out! Was trying to import from oa-components from the component package |
| @@ -0,0 +1,16 @@ | |||
| export interface IUploadedFileMeta { | |||
There was a problem hiding this comment.
Let me know your thoughts on this! The type is important, but since components are hosted in a separate package, I couldn't access the type in the src/pages package.
There was a problem hiding this comment.
That type hasn't been updated in years, so it feels like there won't be too much maintenance burden between the two identical types
There was a problem hiding this comment.
Adding thoughts, I'd say two options:
- [Speed is the way] cheap and quick duplicate and leave a comment about other existing matches.
- [Clean clean clean] there seems to be a shared folder which builds to oa-shared package, which in turn can be used everywhere. This would probably be a good place to put it as there already are some interfaces.
Note: the IImageGalleryItem looks almost exactly like this interface. Extra point to put to shared and creating something like a common interface like FileMeta and extending other ones.
*also I assume you were talking about the src/stores/storage.tsx - IUploadedFileMeta matching interface, if there are even more - more points towards "Clean clean clean".
But would wait for some long time contributors to voice their thoughts too :)
There was a problem hiding this comment.
We're generally on a "clean, clean, clean" path atm. Speed lead approaches can be valid but that on this project has created too many issues as contributors change so the areas with technical debt/issues is knowledge not passed on. And yes, typings to 'oa-shared' is the current direction.
#3789 does a lot of type updates, which we're about to merge in and will likely cause a bunch of conflicts with this branch, apologies.
mchen10
force-pushed
the
3858-elevate-download-with-donation
branch
from
7922e02
to
3c825f8
Compare
mchen10
force-pushed
the
3858-elevate-download-with-donation
branch
from
3c825f8
to
4496a19
Compare
onearmy-community-platform
|
||||||||||||||||||||||||||||
| Project |
onearmy-community-platform
|
| Branch Review |
pull/3869
|
| Run status |
|
| Run duration | 04m 30s |
| Commit |
|
| Committer | Michael |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
65
|
| View all changes introduced in this branch ↗︎ | |
| themeStoreDonationProps={{ | ||
| body: donationThemes?.body, | ||
| iframeSrc: donationThemes?.iframeSrc, | ||
| imageURL: donationThemes?.imageURL, | ||
| }} |
There was a problem hiding this comment.
theme is accessible in the component library, so I don't think this is necessary.
There was a problem hiding this comment.
I'm not sure the right component has been updated here. DownloadFileFromLink certainly isn't right as it includes files now as well as links. I was quite happy with the naming of DownloadWithDonationAsk - does what it says on the tin.
| import { DownloadCounter } from '../DownloadCounter/DownloadCounter' | ||
| import { DownloadStaticFile } from '../DownloadStaticFile/DownloadStaticFile' | ||
|
|
||
| import type { IThemeStoreDonationProps, IUploadedFileMeta } from './types' |
There was a problem hiding this comment.
IUploadedFileMeta can definitely be imported from shared.
| fileLink: string | undefined | ||
| files: (IUploadedFileMeta | File | null)[] | undefined |
There was a problem hiding this comment.
As one or the other will be provided. Both should be optional.
|
Hey @mchen10, how are you getting on? |
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The donation modal only shows up in one place in the app that uses
DownloadWithDonationAsk.What is the new behavior?
We want to start showing the modal everywhere where files are downloaded, so I'm elevating
DownloadWithDonationAskto the component level. Immediately, this means that for research update downloads, we will show the donation modal.Screen.Recording.2024-09-26.at.8.11.27.PM.mov
Does this PR introduce a breaking change?
Git Issues
Closes #3858
What happens next?
Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)
If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.
If you need more immediate feedback you can try reaching out on Discord in the Community Platform
developmentchannel.