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

Using html #2

Closed
mifi opened this issue Nov 5, 2016 · 12 comments
Closed

Using html #2

mifi opened this issue Nov 5, 2016 · 12 comments

Comments

@mifi
Copy link

mifi commented Nov 5, 2016

Nice module!

Would be nice to have the option to put HTML inside the attributes (like description)

@rhysd
Copy link
Owner

rhysd commented Nov 5, 2016

Could you show me some code snippet example? Let's discuss based on code.

@mifi
Copy link
Author

mifi commented Nov 5, 2016

Example:

openAboutWindow({
  icon_path: join(__dirname, 'icon.png'),
  description: '<b>Some bold test</b><a href="myawesomeapp.com">See more here</a>.'
                            })

Currently it just renders the html markup as text, because in the code you're using innerText (https://github.com/rhysd/electron-about-window/blob/master/src/renderer.ts#L30)
Could maybe use innerHTML instead.

@rhysd
Copy link
Owner

rhysd commented Nov 5, 2016

Thanks. I think implementation is very simple as you said. Let me think if there is no risk about XSS because of setting inner HTML.

@rhysd
Copy link
Owner

rhysd commented Nov 5, 2016

I noticed that your example doesn't work. <a> tag navigates to your project page in the 'About Me' window. The window is not protected with sandbox. So we should not navigate to any external page in the window. We need to open the page in external browser.

Regarding to your use case, is it also acceptable to add 'See more' link to description?

@rhysd
Copy link
Owner

rhysd commented Nov 5, 2016

I added a link to application's homepage to application's name instead of adding 'see more' to description because longer description may break the layout. Change is 1e0f5e6.

Does it meet your usecase?

@mifi
Copy link
Author

mifi commented Nov 6, 2016

Oh I see. That sounds like a nice addition too (the link.)

Actually the link wasn't a particular use case, more an example. It was more that it would be nice to be able to add custom HTML block, if someone needs to write some more text with formatting in their about page.

@rhysd
Copy link
Owner

rhysd commented Nov 6, 2016

OK, I think we can use will-navigate event to prevent navigation and open the URL with external browser. I'll try to implement

@rhysd
Copy link
Owner

rhysd commented Nov 6, 2016

openAboutWindow({
   icon_path: join(__dirname, 'icon.png'),
   copyright: 'Copyright (c) 2015 rhysd',
   description: 'Hello, world <a href="https://github.com" class="link">(see more)</a>'
})

now opens a window like below:

2016-11-07 2 28 12

I added will-navigate hook and new-window hook. So <a> tag will open the link with external browser.

I published v1.5.0.

@rhysd rhysd closed this as completed Nov 6, 2016
@mifi
Copy link
Author

mifi commented Nov 6, 2016

Great! thanks! However I think changing from innerText to innerHtml is a breaking change, so we should either bump version to 2.0.0 or introduce a flag in order to use innerHtml instead of innerText.
Maybe introduce a property called "useHtml" ?

@rhysd rhysd reopened this Nov 6, 2016
@rhysd
Copy link
Owner

rhysd commented Nov 6, 2016

Hmm... I think adding new property is better because innerHTML is dangerous. In the case, current behavior is not broken. I'll unpublish 1.5.0 and add useInnerHTML option in 1.6.0.

@mifi
Copy link
Author

mifi commented Nov 7, 2016

You can have useInnerHtml property in 1.5 as well, as long as it defaults to innerText, because then it is backwards compatible :)

@rhysd
Copy link
Owner

rhysd commented Nov 7, 2016

Following semantic versioning, we need to up minor version because it's a new feature without breaking change. Anyway, I already published v1.6.0 👍

@rhysd rhysd closed this as completed Nov 7, 2016
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

No branches or pull requests

2 participants