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

Welovesvg #37

Merged
merged 12 commits into from
Dec 2, 2017
Merged

Welovesvg #37

merged 12 commits into from
Dec 2, 2017

Conversation

NQNStudios
Copy link
Collaborator

Addresses issue #9

Changes proposed by this pull request:

  • Compiler.ts modified to rewrite external links, appending HTML defined in fractive.json
  • The basic template includes Font Awesome
  • Both examples show how to define an external link icon in fractive.json

Notes:

  • I tried to implement this with an SVG icon using Webicon and We Love SVG, but the script tags both projects say to include in the HTML, are dead links.
  • Maybe fractive.json isn't a good place to be defining raw HTML stuff. Or maybe it is, but those things should be specially named aliases.
  • I've laid the groundwork for fractive.json to also define raw HTML for a back button (Back Button #36). If this approach is accepted I can carry on implementing that.

@invicticide to review

Copy link
Owner

@invicticide invicticide left a comment

Choose a reason for hiding this comment

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

I'm not sure the change in Compiler.RewriteLinkNode at 928, under the comment "For macro links, attributes become data attributes", is the cleanest way to handle this. Instead, I'd suggest we just take the dataAttrs argument as-is, and prepend data- back where RewriteLinkNode is being called (or not, as in the case of RewriteExternalLinkNode). The current test if(!hasExternalUrl) made me do a double-take because it doesn't appear here to have anything at all to do with data attributes; this only made sense after I dug around and found out that RewriteExternalLinkNode is building a dataAttrs array that contains href and target.

Separately, I think the externalLinkHTML and backButtonHTML settings could be wrapped in an object for organization, so we get something like linkTags.external, linkTags.inline (another case I've thought of showing an icon; see below), and linkTags.back.

The motivation for tagging inline links is the same as the motivation for tagging external ones: neither link type navigates to a new section, which means that (for most cases) neither link type is a "meaningful" action in terms of game advancement; they're either "let's take a detour to this other website in parallel" or "let's get a little more information about this part of the current section".

Is the FontAwesome-enabled tag for externalLinkHTML what it is because of a requirement of FontAwesome? It's a pretty awkward-looking tag. :P

@NQNStudios
Copy link
Collaborator Author

I like your logic because of all the times I've been playing a Twine game and wanted to make sure to click on all the inline links first. To take it another step further, we could also add an option to tag section links.

As for your last question, yup.

@NQNStudios
Copy link
Collaborator Author

Requested changes (+ function and section link tags) are now implemented, plus an example to demonstrate them without cluttering up the basic examples with a million icons.

@NQNStudios
Copy link
Collaborator Author

One more thing I'd like to change: I can imagine use-cases where it would be better to have a link tag prepended to the text instead. For example, if someone is making a JRPG action menu and the links are function links, it would look better to have the tag in front. I can implement a switch pretty quickly I think.

@invicticide invicticide merged commit ee1a399 into invicticide:dev Dec 2, 2017
@NQNStudios NQNStudios deleted the welovesvg branch December 6, 2017 05:22
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.

2 participants