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

multiple pages: add homepages #2660

Merged
merged 18 commits into from
Jan 30, 2019
Merged

multiple pages: add homepages #2660

merged 18 commits into from
Jan 30, 2019

Conversation

sbrl
Copy link
Member

@sbrl sbrl commented Dec 20, 2018

Finally! Got there in the end. Thanks for the assistance, @waldyrious :D

I ended up cherry-picking the commit from master, remembering that a cherry pick will change the commit hash.

For #2649.

@sbrl sbrl added page edit Changes to an existing page(s). mass changes Changes that affect multiple pages. labels Dec 20, 2018
@owenvoke
Copy link
Member

owenvoke commented Dec 21, 2018

Nice! Didn't know we supported this.

When I render this on Mac, the link doesn't seem to render:

@agnivade
Copy link
Member

When I render this on Mac, the link doesn't seem to render:

Yeah .. I think the node client thinks of it as markdown and removes anything in <>

@waldyrious
Copy link
Member

Nice! Didn't know we supported this.

There's been extensive discussion in #2649, check it out for more context :)

the node client thinks of it as markdown and removes anything in <>

That's valid markdown, though, isn't it? Or does the node client not follow CommonMark?

@agnivade
Copy link
Member

It is actually the : which is causing this. Not sure why. Anybody is welcome to investigate.

@waldyrious
Copy link
Member

It is actually the : which is causing this. Not sure why. Anybody is welcome to investigate.

Huh, that's weird. I would have assumed that it's some sort of html santitization pass that could be removing non-whiteslisted "tags". How did you conclude that the issue is the :?

@agnivade
Copy link
Member

By removing things from Homepage: <http://example.org> until the text rendered. 😛

@sbrl
Copy link
Member Author

sbrl commented Dec 26, 2018

What's blocking this from being merged? Did I forget something

@owenvoke
Copy link
Member

@sbrl, it's broken on the Node client for me still.

@sbrl
Copy link
Member Author

sbrl commented Dec 27, 2018

I see, @pxgamer. In that case I think we're waiting on @agnivade then

@agnivade
Copy link
Member

It seems like @jedahan has already done that. It is just pending some finishing touches.

@waldyrious
Copy link
Member

@sbrl make sure to also address the NPM URL as discussed in the inline comments above :)

@sbrl
Copy link
Member Author

sbrl commented Dec 28, 2018

Done, @waldyrious 😺

@waldyrious
Copy link
Member

I might suggest removing the slashes for the root-type URLs (<https://foo.com/> --> <https://foo.com>), just so they're a tad more readable.

@sbrl
Copy link
Member Author

sbrl commented Dec 30, 2018

Interesting idea, @waldyrious!

I think I prefer the trailing slash though, as the url doesn't look complete without it.

@waldyrious
Copy link
Member

I think I prefer the trailing slash though, as the url doesn't look complete without it.

I understand; I just think it looks inconsistent, since the github repo and npm package URLs don't contain slashes at the end, even though they don't point to a specific file like the URLs ending in .html do. I think we should either add it consistently, or remove it consistently. WDYT?

@sbrl
Copy link
Member Author

sbrl commented Jan 1, 2019

Didn't notice that! I suggest we add them consistently.

@principis
Copy link
Contributor

I just read #2649
This is the first thing I've seen about this, why isn't this change mentioned on the wiki or somewhere else? It would break practically every client.

@agnivade
Copy link
Member

agnivade commented Jan 7, 2019

Any text is allowed in the command description. So we are not breaking any rules.

What might break is the rendering of the text. We are waiting for a fix in node client before we merge this.

@principis
Copy link
Contributor

principis commented Jan 7, 2019

Any text is allowed in the command description. So we are not breaking any rules.

What might break is the rendering of the text. We are waiting for a fix in node client before we merge this.

Yes sure, I just think it should've been mentioned somewhere, it's annoying for clients to suddenly notice that something is broken. :) But I know it's difficult to go and notify everyone. Maybe we should add a clear specification on the wiki? Or maybe a specific page of upcoming changes to the spec. (I know the contributing guide exists).
Anyway I've fixed it. (Not that anyone but myself uses my client)

@agnivade
Copy link
Member

agnivade commented Jan 7, 2019

Maybe we should add a clear specification on the wiki?

What do you want to mention ? A text like this might as well be on any page. So then, by that logic, all clients are already broken.

There no change in the spec here. We are just adding another line in the sub-heading. If that breaks any client, it was already broken in the first place.

@principis
Copy link
Contributor

@agnivade Nope you're right sorry. I was confused about which I thought isn't valid markdown but it is... It's just that the spec for pages is a bit loose and up to interpretation. Maybe I sounded a bit rude which wasn't my intention.

Different question. If we add a "homepage" url do we always link to the homepage or sometimes just to the documentation?
For example: https://docs.ansible.com/ansible/latest/cli/ansible-galaxy.html
Or another example git-add https://git-scm.com/docs/git-add

@agnivade
Copy link
Member

agnivade commented Jan 7, 2019

Different question. If we add a "homepage" url do we always link to the homepage or sometimes just to the documentation?

See my comment #2649 (comment)

Feel free to discuss on that issue if you have more input. Let's keep only PR related discussion here.

@sbrl
Copy link
Member Author

sbrl commented Jan 7, 2019

@principis We're drafting a client spec over in #1065 if you're interested!

@agnivade
Copy link
Member

@sbrl - Feel free to merge this if there are no more pending tasks

@sbrl
Copy link
Member Author

sbrl commented Jan 30, 2019

Thanks, @agnivade! I don't think there are.

@sbrl sbrl merged commit a19866e into master Jan 30, 2019
@owenvoke owenvoke deleted the add-homepages branch January 30, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mass changes Changes that affect multiple pages. page edit Changes to an existing page(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants