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

Copy as png #366

Merged
merged 24 commits into from
Apr 4, 2023
Merged

Copy as png #366

merged 24 commits into from
Apr 4, 2023

Conversation

davidfig
Copy link
Collaborator

@davidfig davidfig commented Mar 18, 2023

Implements #159

  • copy to clipboard from menu
  • improve menu UI
  • add to palette
  • bug: only works at 100%
  • should only include cells and formatting (not gridlines and cell type outlines)
  • Add a max export size and visible error when failing
  • Always export at same resolution per cell

@aws-amplify-us-west-2
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-366.de5w5iglj13on.amplifyapp.com

@davidfig davidfig marked this pull request as ready for review March 18, 2023 23:05
@davidkircos
Copy link
Collaborator

This is a sweet feature! Nice.

Couple pieces of feedback

  1. if the section is off screen the PNG only contains what is on screen.
  2. I think we should not render grid lines. Have it export like presentation mode.

@jimniels
Copy link
Collaborator

jimniels commented Mar 21, 2023

Edit: updated to only reflect functional changes. I will make UI changes

This is very cool!

Here's a few things that popped out to me after looking at it:

  1. If you have the "more" option selected and you close the formatting menu, then make a new selection, the formatting bar pops up again in the same state (it should close the "Copy as PNG" fly out menu)

Mar-21-2023 15-42-52

  1. I think the way this feature works should be uniform regardless of zoom level. For example, these are two screenshots I got using the feature that had the same selection but I was zoomed way out and zoomed way in. Seems like "Copy selection as PNG" should always copy the thing uniformly at 100% regardless of what the user's zoom in the application is.

CleanShot 2023-03-21 at 16 14 51@2x

Here's another example where I zoomed way out and put content way in the upper left and way in the bottom right then made a selection and did "copy as png" and it seemed to export a transparent background?

CleanShot 2023-03-21 at 16 19 11@2x

@jimniels
Copy link
Collaborator

I updated UI styles so that:

  1. The formatting bar has the same style (e.g. color) as the icons in the header and the flyout menu has the same styles as the formatting bar
  2. The "more..." button is horizontal
  3. Sentence casing in the tooltip

Before:

CleanShot 2023-03-22 at 11 41 37@2x

After:

CleanShot 2023-03-22 at 11 41 48@2x

@jimniels
Copy link
Collaborator

Updated the command palette UI with a new icon and sentence casing.

Before:

CleanShot 2023-03-22 at 13 50 06@2x

After:

CleanShot 2023-03-22 at 13 49 41@2x

Also updated the formatting flyout menu to use the same icon:

CleanShot 2023-03-22 at 13 52 43@2x

@davidkircos
Copy link
Collaborator

The screenshot includes content from bordering cells. The frame should go precisely to the selected cells.

image

@davidfig
Copy link
Collaborator Author

I'm not sure I understand the 100% comment. Regardless of zoom level, it always copies to the same DPI. (I fixed a bug where it was clipping when some of the selection was offscreen, which was unrelated.) Do you want the zoom level to change on the png based on the screen zoom? That would feel weird.

@jimniels
Copy link
Collaborator

Regardless of zoom level, it always copies to the same DPI

Yup, that's what I would expect 💯

Copy link
Collaborator

@davidkircos davidkircos left a comment

Choose a reason for hiding this comment

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

Exports are the same resolution per unit area. So in large exports you can zoom in and the content looks good at a cell level.

@jimniels
Copy link
Collaborator

I updated the context menu to use the default icon size (rather than small).

This helps with the flyout "Copy as PNG" menu from looking disabled (it was using the same color as all the other menus in the app, but because it was smaller — small icon, 14px type — than the other menus – default icon, 16px type – it had the look of being disabled.

CleanShot 2023-03-27 at 11 12 38@2x

Now the floating context menu uses the same icons as elsewhere in the app. I think this is an improvement overall.

CleanShot 2023-03-27 at 11 17 08@2x

@davidkircos davidkircos merged commit 0e73eab into main Apr 4, 2023
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