Skip to content

Conversation

dtrudg
Copy link
Contributor

@dtrudg dtrudg commented Mar 19, 2018

Hi @vsoch - I think this is all that's needed to have the site directing people to the new 2.4.5

Would you be able to review these for an update to the site? Thanks!

@dtrudg dtrudg requested a review from vsoch March 19, 2018 22:04
@vsoch
Copy link
Member

vsoch commented Mar 19, 2018

yeah! Let me give it a quick look.

@vsoch
Copy link
Member

vsoch commented Mar 19, 2018

The links to files look good - let's add links to both @jtriley and the two PRs mentioned so the user can follow easily to learn more. You should be able to use {{ site.repo }} to refer to the start of the github links, in case they change in the future.

Copy link
Member

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

  • add link to @jtriley reference
  • add links to two PRs requested using {{ site.repo }}

@vsoch
Copy link
Member

vsoch commented Mar 19, 2018

And congrats on the release! 💪

@vsoch
Copy link
Member

vsoch commented Mar 19, 2018

Q: Why was @dctrud excited to go to the market?
A: They were having a deal on ice cream! It was 2.4.5 :)

@dtrudg
Copy link
Contributor Author

dtrudg commented Mar 20, 2018

@vsoch - I think ^^ should do it.

@vsoch
Copy link
Member

vsoch commented Mar 20, 2018

ok taking a look! The text looks spot on, just want to make sure!

@vsoch
Copy link
Member

vsoch commented Mar 20, 2018

ah one more detail, and this is my fault because I just forgot about it (and one of my biggest pet peezes with websites!). The link icon that is being used says "this opens externally" but the links here don't, so you would want to add a class "no-after" to them, like this:

<a href="https://github.com/singularityware/singularity/pull/1387" class="no-after">1387</a>/<a class="no-after" href="https://github.com/singularityware/singularity/singularity/pull/1397">1397</a>

and for jtriley

<a href="https://github.com/jtriley" class="no-after">@jtriley</a>

OR if you want to keep the external link icon (and make it an external link, which is also a good idea) just add the target attribute to be _blank:

<a href="https://github.com/jtriley" target="_blank">@jtriley</a>

@dtrudg
Copy link
Contributor Author

dtrudg commented Mar 20, 2018

Hi @vsoch - I'm kinda confused about these link class requests. I wrote the new links in markdown, so I don't have the raw html to insert a class into, and I think the icons are correct for the links.

An external link is one on a different site/domain. not something that has target _blank.

External Links are hyperlinks that point at (target) any domain other than the domain the link exists on (source).

In layman's terms, if another website links to you, this is considered an external link to your site. Similarly, if you link out to another website, this is also considered an external link.

https://moz.com/learn/seo/external-link

These links are at GitHub, and we are lbl.gov so it's correct that they are external links. The '_blank' target isn't an external link... it's just to open a link (external or not) in a new window or tab.

@vsoch
Copy link
Member

vsoch commented Mar 20, 2018

It's a link that opens in a new window. The html can render in the markdown file okay, give it a try! And the class already exists - I made it for this exact purpose :)

@dtrudg
Copy link
Contributor Author

dtrudg commented Mar 20, 2018

I completely disagree here - the definition of an external link is not one that opens in a new window. - it is one that links to a different domain. There has been convention on some sites to open external links in a new window, but that's not related to the distinction between an external or internal link.

Also - it is really weird to write raw HTML in markdown for links when there is markup for it.

I will make the change though to have the links target _blank though in the interests of getting this doc up ASAP.

Copy link
Member

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

Sorry I misspoke, I meant external tab! I don't like link that open a new windows either. It is frustrating however when you are reading a site and you open something that you think will open a new tab and you completely lose the page that you were reading. That's more along the lines that I'm thinking of here!

@dtrudg
Copy link
Contributor Author

dtrudg commented Mar 20, 2018

Ok, cool - preference noted. I'm a 'more than 5 tabs open is oooof' kinda guy, and tend to dislike links targetting new windows unless I ask for it with Ctrl.

Fingers crossed the html is good now, so we can get the 2.4.5 reference up in the docs, since it's a security release. Cheers!

Copy link
Member

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

hey there is still a small typo - I know this is important and you already responded so many times! Let me pull it and I'll fix it up and get it merged asap :) I hope you went to sleep!

This is a security-related point release, bringing the following fix thanks to Justin Riley ([@jtriley](https://github.com/jtriley)):

PR<a target="_blank" href="{{ site.repo }}/pull/1387">1387</a>/<a target="_blank" href="({{ site.repo }}/singularity/pull/1397">1397</a> - python: strip "Authorization" header on (urllib) redirects to different domains

Copy link
Member

Choose a reason for hiding this comment

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

This second link might render to two Singularity?

@vsoch vsoch merged commit ff03f56 into singularityware:master Mar 20, 2018
@vsoch
Copy link
Member

vsoch commented Mar 20, 2018

Looks great @dctrud ! Thank you! http://singularity.lbl.gov/release-2-4-5

@vsoch
Copy link
Member

vsoch commented Mar 20, 2018

oh and uh, if you ever see my computer, maybe don't look too closely at the number of tabs...
image :)

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