-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
[TASK-884] Implement reusable export button #5191
base: main
Are you sure you want to change the base?
Conversation
cd9a7f1
to
69b1102
Compare
setIsMessageOpen(true); | ||
}) | ||
.catch(() => { | ||
alertify.error(t('There was an error exporting the data')); |
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.
I would try to use our robust handleApiFail
(from api.ts
file) function here. It handles all the edge cases :)
new Promise<void>((resolve, reject) => { | ||
// Simulates backend export process. | ||
setTimeout(() => { | ||
if (Math.random() > 0.5) { |
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.
I like this simulation 👍
|
||
export default function ExportToEmailButton({ | ||
exportFunction, | ||
...props |
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.
I wonder if we ever want to expose anything else besides label
… I feel like it would be more clear what this does if we do. With that spread we invite people to visit button.tsx
to see what is there to be set (and in reality we only want people to set label
). IIRC the Figma designs suggest that all current usages of this button would look identical. What do you think?
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.
If we end up only exposing label
, than the stories file needs to only expose it too :)
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.
At first I exposed only label, but then I thought someone could want to chnage other properties from the button, like the variant or size, according to the needed layout, so I changed the implementation to allow for more control.
Do you think it's better to change expose label for now and, if needed, someone can extend it in the future?
</KoboPrompt> | ||
); | ||
|
||
export default function ExportToEmailButton({ |
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.
I know that the name is almost self-explanatory, but since you're working on this right now, could you describe this in a JSDoc? Nothing very detailed. Just 1-2 paragraphs for future archeologist that will help out getting a gist of what this is.
]} | ||
> | ||
<div> | ||
<p> |
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.
Unfortunately <p>
has margins (default browser) and it shouldn't have. If you'd remove that <div>
there are styles in .kobo-modal__content
that handles that :)
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.
And if you take a look at KoboPrompt
, then you'll see that we are wrapping props.children
inside a wrapper :)
import buttonStory from '../common/button.stories'; | ||
|
||
export default { | ||
title: 'common/ExportButton', |
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.
Please change it to misc/ExportButton
, to not put it in the "common" section.
Task
Checklist
./python-format.sh
to make sure that your code lints and that you've followed our coding styleDescription
This PR implements the functionality for the export button.
Notes
async
function to be called when clickedalertify
is used to display an error messageTesting
?ff_activityLogsEnabled=true
feature flag onActivity
sub view