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

[frontend] Add quick export button in Report overview (#2515) #4831

Merged
merged 32 commits into from Nov 8, 2023

Conversation

lndrtrbn
Copy link
Member

@lndrtrbn lndrtrbn commented Nov 2, 2023

Proposed changes

  • Add a quick button to be able to export a Report directly from Overview page
  • After export starts: redirect to Content page and display the exported PDF when it is ready

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@lndrtrbn lndrtrbn added the filigran team use to identify PR from the Filigran team label Nov 2, 2023
@lndrtrbn
Copy link
Member Author

lndrtrbn commented Nov 2, 2023

About the file TasksList: All diffs are due to converting class component into functionnal component

@SouadHadjiat
Copy link
Member

A few feedback during my testing :

  1. When there is no export connectors available, the button is disabled, but doesn't look like it, it looks active, but I just can't click on it.
    Capture d'écran 2023-11-02 161616

  2. When I ran export-file-stix connector only, the quick export button was enabled, I clicked on it and I could export the file in json
    Capture d'écran 2023-11-02 161707

  3. Then when I ran the export, I was redirected to the content view, but the file never appeared (since it's json, and not pdf)
    Capture d'écran 2023-11-02 161910

@Kedae
Copy link
Member

Kedae commented Nov 2, 2023

Then when I ran the export, I was redirected to the content view, but the file never appeared (since it's json, and not pdf)

Yes I think we mentionned that in the future we could enrich the Content view with all exported files. We will rediscuss that after this chunk

@lndrtrbn
Copy link
Member Author

lndrtrbn commented Nov 2, 2023

When there is no export connectors available, the button is disabled, but doesn't look like it, it looks active, but I just can't click on it.

Indeed need to fix this little style issue

When I ran export-file-stix connector only, the quick export button was enabled, I clicked on it and I could export the file in json

And this one also

Copy link
Member

@SarahBocognano SarahBocognano left a comment

Choose a reason for hiding this comment

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

The cursor is not right on hover, should be a "pointer". And I can not click on it ?
Capture d'écran 2023-11-03 120216

/>
</Tooltip>

Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree on this one as it separate two distinct blocs of the DOM

/ task.task_expected_number)
* 100,
)
}
Copy link
Member

Choose a reason for hiding this comment

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

indentation

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no indentation issue here. Just imbricated ternary conditions that make this aspect

@lndrtrbn
Copy link
Member Author

lndrtrbn commented Nov 3, 2023

@SouadHadjiat about your 3 points :

  1. style added, resolved.
    2&3. should be resolved also with our code update.

@SarahBocognano about button with no pointer cursor: it's because you have no connector to export pdf started and so the button is disabled. Yes it should be grey, done, point 1 of Souad 😁

@SarahBocognano
Copy link
Member

Otherwise, everything works fine after the updates ! :) ✅

@CelineSebe CelineSebe merged commit d7841a1 into master Nov 8, 2023
6 checks passed
Goumies pushed a commit that referenced this pull request Nov 9, 2023
Co-authored-by: CelineSebe <celine.sebe@filigran.io>
@CelineSebe CelineSebe deleted the issue/2515b branch November 17, 2023 08:25
@CelineSebe CelineSebe added the solved use to identify issue that has been solved (must be linked to the solving PR) label Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team solved use to identify issue that has been solved (must be linked to the solving PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants