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

New menu #70

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

New menu #70

wants to merge 7 commits into from

Conversation

TonyLianLong
Copy link
Contributor

@TonyLianLong TonyLianLong commented Mar 2, 2020

This fixes #69.

@davidanthoff
Copy link
Member

Cool! I'm testing on different platforms.

On Windows, I get:

image

  • I think we don't need the whole top-level Window menu, can you remove that?
  • Can you remove the dev tool window item? I don't think we should expose that to normal users.
  • I wonder whether we could enable/disable the zoom menu items depending on whether the active plot is one where these commands work? If that works we could also remove the text (For Static Images) because that would be kind of obvious in that case, right?

Everything else seems to work :) I'll test on Linux and Mac next.

@davidanthoff
Copy link
Member

Here are my Mac comments:

  • There is a menu Window -> Window, that should probably be removed?
  • Could we rename the main menu (the one in bold) from Julia to ElectronDisplay?
  • The Edit menu has a submenu speech? Does that do anything?
  • There is also a "Start dictation" and emoji menu, do these do anything?

@davidanthoff
Copy link
Member

And Linux:

  • There is also a Window menu that I think can go entirely.

@davidanthoff
Copy link
Member

Oh, and for some reason tests fail? Probably also needs some update to fix that?

@TonyLianLong
Copy link
Contributor Author

TonyLianLong commented Mar 14, 2020

Windows:

  • I think we don't need the whole top-level Window menu, can you remove that?

Removed

  • Can you remove the dev tool window item? I don't think we should expose that to normal users.

Removed

  • I wonder whether we could enable/disable the zoom menu items depending on whether the active plot is one where these commands work? If that works we could also remove the text (For Static Images) because that would be kind of obvious in that case, right?

It's possible, but it's hard and it's likely the code will be messy because this is dynamic in plotgallery.

@TonyLianLong
Copy link
Contributor Author

TonyLianLong commented Mar 14, 2020

Mac:

  • There is a menu Window -> Window, that should probably be removed?

Yes.

  • Could we rename the main menu (the one in bold) from Julia to ElectronDisplay?

It depends on Electron.jl which generates the application.

  • The Edit menu has a submenu speech? Does that do anything?

That's Mac's. Most if not all windows have them. I can't remove them in the code.

  • There is also a "Start dictation" and emoji menu, do these do anything?

Same.

Linux menu has been dealt while removing the Window menu.

@TonyLianLong
Copy link
Contributor Author

Problem with changing the name in menu for mac: https://discuss.atom.io/t/change-app-name-in-menu-bar/39839

@TonyLianLong
Copy link
Contributor Author

I have looked into test.jl. It does not work because the stringmime in vega.jl returns Base64, which is unwanted. I tried replacing stringmime with repr, but it does not work either. I'm not familiar with this part. Can you look into it? @davidanthoff

@TonyLianLong
Copy link
Contributor Author

Should I quote the base64 image with quotes in line 329 and 338 of ElectronDisplay.jl? I mean change them to displayplot(d, "image", "'" + imgdata + "'").

@davidanthoff
Copy link
Member

@TonyLianLong I added some methods in a commit in this branch that I believe should fix the base64 problem, i.e. with those methods I think stringmime should now just return text, if I understand this stuff properly. Can you give it a try?

@TonyLianLong
Copy link
Contributor Author

TonyLianLong commented Apr 13, 2020

Now these tests run successfully on my computer. Although not all plots are able to load on the window, there is no error on the Julia side (this could be an issue with fetch API on the JavaScript side, which we are currently not fixing). Let's see what will happen on CI.

@TonyLianLong TonyLianLong marked this pull request as ready for review April 19, 2020 00:55
@TonyLianLong
Copy link
Contributor Author

@davidanthoff The tests are successful, but some are cancelled after being successful. Does that mean I still have some code in the test files need to fix, or it is caused by Github Actions?

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.

Sort out menu situation
2 participants