-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: Add support for product export in UI #8281
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
onRemove={handleRemove} | ||
readonly={readonly} | ||
/> | ||
{!readonly && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just indentation changes, no actual code changed other than the readonly check.
filters, | ||
readonly, | ||
prefix, | ||
}: DataTableFilterProps) => { | ||
const { t } = useTranslation() | ||
const [searchParams] = useSearchParams() | ||
const [open, setOpen] = useState(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Can't we rely solely on the query parameters for the activeFilters
state? I feel like it should be possible, and we can remove some code around maintaining their state (so basically we have getInitialFilters be the state, and that gets memoized with location.search
passed to it)
f16386d
to
ae11951
Compare
/snapshot-this |
@sradevski, this doesn't seem to work-out-of-the box with the local file provider – did you use S3? The exported CSV file is put into the Update: Perhaps the issue is also related to the fact that the generated URL of the exported file points to a path on the local file system and not one on the server. Not sure if our Vite application can deal with this? |
@olivermrbl That behavior is intentional actually. The problem with using the local file provider for the exports is that we can't have security controls (so, no support for private files) if we put it in static, as anyone would be able to read the file, so I create a Let me know if you think this should behave differently, it's super easy to change it. |
Given the local file provider is only used for development, wouldn't this be OK? |
I've heard people use it in production as well for some small sites, so I thought to err on the safe side. If we are OK with it since people really shouldn't use it in prod, then we can def. put it in You can actually configure if you want it in static or private currently (you just need to set |
Originally (in V1), the provider was supposed to be for development only, and IMO, this should still be the recommendation from our side. With that in mind, I would suggest we optimize for this use case such that we can guarantee a good first experience across all our features. Then in the rare (my hunch; how often have you heard this being used in prod?) case, we can guide developers to a secure configuration with the local provider. Wdyt? |
@olivermrbl it should definitely be only for development, but I've seen at least 2 conversations on Discord, and one from a local agency (and I obviously told them to use S3 😄). Then let me use the same, static directory for private files as well, and leave a comment that if they really wanted to they can change it through the config, WDYT? |
Sounds good! |
ae11951
to
0c20399
Compare
@sradevski, the static folder is correctly used now for the file uploads. However, we still have the issue with the admin dashboard attempting to download from the local file system. This leads to the download containing the |
@kasperkristensen, could you give this a review? 🙏 |
0c20399
to
b22d407
Compare
@olivermrbl I missed removing one check (see last commit), should be good now. |
b22d407
to
9b833b2
Compare
9b833b2
to
0d4099f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@sradevski, works nicely – good one 👍 |
I've keep the PR simple, so it is missing column selection (we should discuss if we event want it now or in the polish stage) and push notifications (similarly, needs discussion if we want to tackle it right away).
One thing we need to improve (outside of the scope here) is to preserve query params on
handleSuccess
call, so filters and such is not lost if you eg. go tocreate
and go back.CLOSES CC-234