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

[TASK-884] Implement reusable export button #5191

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pauloamorimbr
Copy link
Contributor

Task

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Run ./python-format.sh to make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes
  8. Create a testing plan for the reviewer and add it to the Testing section

Description

This PR implements the functionality for the export button.

  • Actual export function is mocked and will not generate any data
  • The mocked results now have a 50% chance of failure for testing purposes

Notes

  • A new "ExportButton" component was created.
  • It receives an async function to be called when clicked
  • If the export process is successfully started it will display a message to the user
  • If an error occurs, alertify is used to display an error message

Testing

  • Access the project settings
  • Be sure to have the ?ff_activityLogsEnabled=true feature flag on
  • Access the Activity sub view
  • Click the export button some times.
  • It should display the successful or error message depending on the (random) result.

Copy link

Copy link

@pauloamorimbr pauloamorimbr force-pushed the task-884-implement-reusable-export-button branch from cd9a7f1 to 69b1102 Compare October 24, 2024 12:30
setIsMessageOpen(true);
})
.catch(() => {
alertify.error(t('There was an error exporting the data'));
Copy link
Member

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) {
Copy link
Member

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
Copy link
Member

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?

Copy link
Member

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 :)

Copy link
Contributor Author

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({
Copy link
Member

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>
Copy link
Member

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 :)

Copy link
Member

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',
Copy link
Member

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.

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.

2 participants