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

docs: add more fiddles for launch in fiddle feature #19849

Closed
wants to merge 16 commits into from

Conversation

cvaldez98
Copy link
Contributor

@cvaldez98 cvaldez98 commented Aug 20, 2019

Description of Change

Add in more fiddles to demonstrate the screen and accelerator module.
This is adding more code examples that can have the Launch in Fiddle feat on electronjs.org .
The code examples are slightly modified to help the user out, but I'm open to changes of course.
Follow up from #19759
Notes: none
cc @MarshallOfSound @codebytere @ckerr @deermichel

cvaldez98 and others added 8 commits August 16, 2019 11:58
Co-Authored-By: Mark Lee <malept@users.noreply.github.com>
Co-Authored-By: Samuel Attard <samuel.r.attard@gmail.com>
Co-Authored-By: Samuel Attard <samuel.r.attard@gmail.com>
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 20, 2019
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

I love this; thank you for the PR!

docs/fiddles/accelerator/main.js Outdated Show resolved Hide resolved
docs/fiddles/screen/fit-screen-external/main.js Outdated Show resolved Hide resolved
docs/fiddles/screen/fit-screen-external/main.js Outdated Show resolved Hide resolved
docs/tutorial/security.md Outdated Show resolved Hide resolved
Copy link
Contributor

@deermichel deermichel left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this :).

Reviewing on iPad sucks😅 – but what i can say rn is that you‘re missing the end backticks of fiddle='url' (instead of fiddle='url).

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 21, 2019
@cvaldez98 cvaldez98 changed the base branch from master to 7-0-x August 21, 2019 22:07
@cvaldez98 cvaldez98 changed the base branch from 7-0-x to master August 21, 2019 22:07
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine with merging this once @deermichel is happy

@zcbenz
Copy link
Member

zcbenz commented Aug 29, 2019

@deermichel Can you take another look of this PR?

@deermichel
Copy link
Contributor

Yes, I'll do that tmw 👍

Copy link
Contributor

@deermichel deermichel left a comment

Choose a reason for hiding this comment

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

Found some last changes.

Additionally on my German keyboard layout, the global shortcuts don't really work like expected: Ctrl doesn't work at all and Y & Z are swapped - but I think that's an Electron problem 😬

});

// and load the index.html of the app.
mainWindow.loadFile('index.html');
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm there is no index.html in this fiddle - we can remove this line or add a simple html

y: externalDisplay.bounds.y + 50
})
win.loadURL('https://electronjs.org')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

how about an else case with a console.log("no ext display or so")?

// Adding a global shortcut to electron
//
// For more info, see:
// https://electronjs.org/docs/api/screen
Copy link
Contributor

Choose a reason for hiding this comment

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

should point to the global shortcut docs :)

@zcbenz
Copy link
Member

zcbenz commented Dec 16, 2019

@cvaldez98 Can you address the review above?

@zcbenz
Copy link
Member

zcbenz commented Jan 15, 2020

I'm closing this PR since there has been no activity for a long time, please let me know if you still want to work on this and I'll reopen this PR.

@zcbenz zcbenz closed this Jan 15, 2020
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.

None yet

4 participants