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.

Sign up for GitHub

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

Jump to bottom

Remove inline JavaScript from header and replace it with Jekyll directive #18

Merged
merged 9 commits into from
Oct 16, 2017
Merged

Remove inline JavaScript from header and replace it with Jekyll directive #18

merged 9 commits into from
Oct 16, 2017

Conversation

Swardu
Copy link
Contributor

@Swardu Swardu commented Oct 15, 2017

Ref: #16
Uses site.url and removes the "https://" part.

@pluehne pluehne self-requested a review October 15, 2017 17:02
@pluehne pluehne self-assigned this Oct 15, 2017
pluehne
pluehne suggested changes Oct 15, 2017
View reviewed changes
Copy link
Contributor

@pluehne pluehne left a comment

Choose a reason for hiding this comment

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

Many thanks for your contribution 😃. I just have one minor formatting remark.

docs/_layouts/default.html Outdated
@@ -35,7 +35,7 @@
</a>
<a href="{{ site.baseurl }}/" alt="Hubble Enterprise">
<img src="{{ site.baseurl }}/assets/images/logo-title.svg" class="project-logo" width="3rem" height="3rem">
<h1 class="project-name"><script type="text/javascript">document.write(gheHostname())</script> Statistics</h1>
<h1 class="project-name">{{ site.url | remove:'https://' }} Statistics</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you plase add a space after the colon and put the https:// part in double quotes for consistency with the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have updated the file with your requested changes.

@pluehne
Copy link
Contributor

pluehne commented Oct 15, 2017

I’m not sure whether it would also be good to entirely get rid of the gheHostname and gheUrl JavaScript functions still used here.

@larsxschneider, @GitHugop: What do you guys think?

@pluehne pluehne changed the title Removing inline JavaScript from header and replacing it with Jekyll Remove inline JavaScript from header and replace it with Jekyll directive Oct 15, 2017
pluehne
pluehne suggested changes Oct 15, 2017
View reviewed changes
docs/_layouts/default.html Outdated
@@ -35,7 +35,7 @@
</a>
<a href="{{ site.baseurl }}/" alt="Hubble Enterprise">
<img src="{{ site.baseurl }}/assets/images/logo-title.svg" class="project-logo" width="3rem" height="3rem">
<h1 class="project-name">{{ site.url | remove:'https://' }} Statistics</h1>
<h1 class="project-name"> {{ site.url | remove:"https://" }} Statistics</h1>
Copy link
Contributor

@pluehne pluehne Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for being ambiguous. I meant that a space should be inserted after remove: like this: remove: "https://" (and not in the beginning of the title). Could you change that?

I know I’m a bit pedantic, but I want to keep the coding style consistent as much as possible 😃.

pluehne
pluehne approved these changes Oct 15, 2017
View reviewed changes
Copy link
Contributor

@pluehne pluehne left a comment

Choose a reason for hiding this comment

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

Many thanks!

@pluehne
Copy link
Contributor

pluehne commented Oct 15, 2017

Let’s wait for @larsxschneider to review and merge this.

@larsxschneider: Do you think it would make sense to also evaluate the server name via Jekyll for the charts.js, as I suggested above?

@Swardu
Copy link
Contributor Author

Swardu commented Oct 15, 2017 •
edited
Loading

You welcome. Regarding the functions, I don't think it's a great idea to mix JavaScript files with Jekyll. Something that can be done is merging both functions into one, like:

function getURL() {
return $(location).prop("protocol") + "//" + $(location).prop("hostname");
}

larsxschneider
larsxschneider requested changes Oct 15, 2017
View reviewed changes
docs/_layouts/default.html Outdated
@@ -35,7 +35,7 @@
</a>
<a href="{{ site.baseurl }}/" alt="Hubble Enterprise">
<img src="{{ site.baseurl }}/assets/images/logo-title.svg" class="project-logo" width="3rem" height="3rem">
<h1 class="project-name"><script type="text/javascript">document.write(gheHostname())</script> Statistics</h1>
<h1 class="project-name">{{ site.url | remove: "https://" }} Statistics</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you 👍 ! Is there a way to handle http:// URLs, too? Some GitHub Enterprise installations use just HTTP 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn’t aware of this, good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing it out.

@Swardu Swardu changed the title Remove inline JavaScript from header and replace it with Jekyll directive [WIP] Remove inline JavaScript from header and replace it with Jekyll directive Oct 15, 2017
larsxschneider
larsxschneider reviewed Oct 15, 2017
View reviewed changes
docs/_layouts/default.html Outdated
@@ -35,7 +35,7 @@
</a>
<a href="{{ site.baseurl }}/" alt="Hubble Enterprise">
<img src="{{ site.baseurl }}/assets/images/logo-title.svg" class="project-logo" width="3rem" height="3rem">
<h1 class="project-name"><script type="text/javascript">document.write(gheHostname())</script> Statistics</h1>
<h1 class="project-name">{{ site.url | remove: "http://", "https://" }} Statistics</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get this error with Jekyll 3.3.1 locally:

  Liquid Exception: Liquid error (line 38): wrong number of arguments (given 3, expected 2) in /_layouts/default.html

Any idea what to do about it?

Copy link
Contributor Author

@Swardu Swardu Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like "remove" only supports 1 argument, there doesn't seem to be another way on the Jekyll docs. We can try using both "remove" and "remove_first".

@Swardu Swardu changed the title [WIP] Remove inline JavaScript from header and replace it with Jekyll directive Remove inline JavaScript from header and replace it with Jekyll directive Oct 15, 2017
@larsxschneider larsxschneider merged commit f86eb88 into Autodesk:master Oct 16, 2017
@larsxschneider
Copy link
Collaborator

Thank you @GitHugop ! 👍 👍 👍

@pluehne Yeah, we could use the same approach in chart.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pluehne pluehne pluehne approved these changes

@larsxschneider larsxschneider larsxschneider approved these changes

Assignees

@pluehne pluehne

Labels
aesthetic enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Swardu @pluehne @larsxschneider