Skip to content

Conversation

@lyjeileen
Copy link
Collaborator

@lyjeileen lyjeileen commented May 25, 2024

Change

  • Use PopoverMenu component to create the action menu (Only supports download now)

Maybe we could consider reduce the min-width of the PopoverMenu component in a follow-up? It looks too wide for the options in Vega-Lite and I couldn't overwrite it because when it's being rendered, it doesn't have any parent container.

Screenshots

Before

Screenshot 2024-05-24 at 5 38 01 PM

After

Screenshot 2024-05-24 at 5 37 29 PM

Video

May-24-2024 17-40-49

@lyjeileen lyjeileen marked this pull request as draft May 29, 2024 16:00
@lyjeileen lyjeileen marked this pull request as ready for review May 30, 2024 00:08
)
.then((url) => {
menuItems[index].href = url
menuItems[index].downloadFileName = fileName
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
menuItems[index].downloadFileName = fileName
menuItems[index].downloadFileName = `${fileName}.${ext}`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is not needed. Depending on the file type, .svg and .png will be added automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its better to mention the type. I see no harm in doing so

@lyjeileen lyjeileen requested a review from Shiti May 30, 2024 16:10
@Shiti
Copy link
Contributor

Shiti commented May 30, 2024

@lyjeileen are you setting the width of the popover somewhere ? From your screenshots and video, it appears to be too wide, i.e., wider than the content.

@lyjeileen
Copy link
Collaborator Author

@lyjeileen are you setting the width of the popover somewhere ? From your screenshots and video, it appears to be too wide, i.e., wider than the content.

I mentioned this in my PR description. PopoverMenu set min-width: 240px for the menu. I can't overwrite it in the VegaLite component because when the menu is being rendered, it doesn't have VegaLite as its parent container. So I thought it would be better to reduce the min-width in the Popover Menu in a follow-up PR.

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.

3 participants