EES-5734 copy download charts #5674
Conversation
| } | ||
| }; | ||
|
|
||
| if (isOpen) { |
There was a problem hiding this comment.
I think we need something so that people can close this by pressing the escape key?
| }, [isOpen]); | ||
|
|
||
| return ( | ||
| <div className={styles.exportDropdown}> |
There was a problem hiding this comment.
In the prototype, the Export button was here:
Realise this is a pain to change, so just checking this new position has been run past Cam/Marv?
There was a problem hiding this comment.
I've had a chat with Cam and we've agreed it's better where it is now although I'm going to move it to the right of the container to make it more visible. Worth mentioning the export stuff wouldn't work where the designs are as the tabs component is shared and used throughout the site for other tabs etc. You'd just keep losing the reference to the chart when you change tabs then the export buttons won't work
| }, [isOpen]); | ||
|
|
||
| return ( | ||
| <div className={styles.exportDropdown}> |
There was a problem hiding this comment.
Maybe add some vertical padding for the ButtonText below?
| <ButtonText onClick={handleDivDownload}> | ||
| Download chart as PNG image | ||
| </ButtonText> | ||
| <br /> |
There was a problem hiding this comment.
Can we change the menu CSS rather than use BRs here?
| const handleDivDownload = useCallback(async () => { | ||
| const jpeg = await getDivJpeg(); | ||
| if (jpeg) { | ||
| downloadFile(jpeg, 'Chart.png'); |
There was a problem hiding this comment.
Is it possible to provide more context for the file name? Although maybe this is acceptable with Cam for now - I guess the downside here is if someone downloads multiple charts it could be confusing for a user, but unsure how often that would happen - maybe a potential future piece of work than something to do now.
There was a problem hiding this comment.
Yer I was thinking that but just decided to work off the AC, will have a word with Cam on it as I'm not really sure what value we'd use for it
| @@ -115,17 +116,21 @@ const ChartBuilderTabSection = ({ | |||
| [onTableUpdate, query, releaseVersionId], | |||
| ); | |||
|
|
|||
| const exportRef = useRef(null); | |||
There was a problem hiding this comment.
So I think this ref is being used by html2canvas to indicate what should be rendered?
I was wondering if there was any way to make it clearer what this ref is being used for - not suggesting a change here as I'm not sure there is a clear way without adding a comment, and then you'd need to leave the same comment in other places ChartBuilder/ChartRenderer are used.
| const handleDivDownload = useCallback(async () => { | ||
| const jpeg = await getDivJpeg(); | ||
| if (jpeg) { | ||
| downloadFile(jpeg, 'Chart.png'); |
There was a problem hiding this comment.
The file seem to be saving as a jpeg, not a png. Assuming there isn't any hard requirement on the image type, I'd rename the extension, variable names, and any link text to JPEG. Or maybe the mime type in useGenerateImage can be changed to image/png.
There was a problem hiding this comment.
it actually always saves as a PNG so I'm not even sure why the library had it named that way. Will rename it so it's obvious
There was a problem hiding this comment.
I checked the type of the images I downloaded locally and they were jpegs
There was a problem hiding this comment.
mm strange the ones I have have PNG as image type - thanks for the heads up I'll look into it
...explore-education-statistics-common/src/modules/find-statistics/components/DataBlockTabs.tsx
Show resolved
Hide resolved
| }, [isOpen]); | ||
|
|
||
| return ( | ||
| <div className={styles.exportDropdown}> |
There was a problem hiding this comment.
Just checking that this includes everything we need for accessibility generally? Might be worth someone who knows these things looking it over
There was a problem hiding this comment.
@mmyoungman just looking at the gov style guide and not really sure what we usually do for toast alerts/messages, just wondering if you've encountered any that can be re-used here?
|
I looked through this with Mark but since then a couple of things have jumped out:
Depending on the required browser support we could look to use the popover api? This would at least handle some of the above for us such as light dismiss, focus handling.
Obviously I'm new to the project so if the above not relevant feel free to ignore! |
|
Changes made:
Added the ability to export chart as PNG or copy charts to clipboard