README suggestions #176

2018-09-12T17:18:31-05:00

thewatts commented

2018-09-12 17:18:31 -05:00

(Migrated from github.com)

Thanks for this awesome project!

I spent some time really digging into the README to learn in the ins/outs of how everything works.

These changes are just some suggestions based on my experience going through the docs.

100% no pressure to use them, but thought they might be helpful.

Thanks for this awesome project! I spent some time really digging into the README to learn in the ins/outs of how everything works. These changes are just some suggestions based on my experience going through the docs. 100% no pressure to use them, but thought they might be helpful.

robinnorth commented

2018-09-20 04:01:39 -05:00

(Migrated from github.com)

Hey @thewatts, thanks for your very extensive pull request. I'm sorry it's taken a while for someone to get back to you about it!

On a quick first read through, it seems like there are some helpful additions and amendments. I will try and find some time a bit later to give it a full review, as there are a lot of changes 😉

Hey @thewatts, thanks for your very extensive pull request. I'm sorry it's taken a while for someone to get back to you about it! On a quick first read through, it seems like there are some helpful additions and amendments. I will try and find some time a bit later to give it a full review, as there are a lot of changes 😉

robinnorth (Migrated from github.com) requested changes 2018-09-20 12:51:49 -05:00

robinnorth (Migrated from github.com) left a comment

@thewatts, by and large all of these changes look great to merge! You've made some nice improvements here.

I've just made a few comments which are mostly concerned with readability and style which would be great to action, if you could.

We also need to see what @BehindTheMath has to say, as they are the most active maintainer of this library.

@thewatts, by and large all of these changes look great to merge! You've made some nice improvements here. I've just made a few comments which are mostly concerned with readability and style which would be great to action, if you could. We also need to see what @BehindTheMath has to say, as they are the most active maintainer of this library.

README.md

robinnorth (Migrated from github.com) commented

2018-09-20 12:20:29 -05:00

Something like '...to make users feel like they are browsing an app, especially for those with low bandwidth connections.' would read a bit better here, I think

Something like _'...to make users feel like they are browsing an app, especially for those with low bandwidth connections.'_ would read a bit better here, I think

robinnorth (Migrated from github.com) commented

2018-09-20 12:25:37 -05:00

After testing against the latest build, it looks like this has crept up to 6kb

robinnorth (Migrated from github.com) commented

2018-09-20 12:27:58 -05:00

'Allows page transitions with CSS animations'

_'Allows page transitions with CSS animations'_

README.md

robinnorth (Migrated from github.com) commented

2018-09-20 12:30:22 -05:00

'non-HTML'

_'non-HTML'_

README.md

robinnorth (Migrated from github.com) commented

2018-09-20 12:30:47 -05:00

'non-HTML'

_'non-HTML'_

README.md

						
				@@ -335,30 +395,29 @@ var pjax = new Pjax({

				  }

robinnorth (Migrated from github.com) commented

2018-09-20 12:37:15 -05:00

'Here is some CSS that works well with the above configuration:'

_'Here is some CSS that works well with the above configuration:'_

robinnorth (Migrated from github.com) commented

2018-09-20 12:37:54 -05:00

'...when positioning absolutely'

_'...when positioning absolutely'_

README.md

						
				@@ -402,7 +463,7 @@ To give context to this CSS, here is an HTML snippet:

				<!doctype html>

robinnorth (Migrated from github.com) commented

2018-09-20 12:39:06 -05:00

'...Pjax attempts...'

_'...Pjax attempts...'_

robinnorth (Migrated from github.com) commented

2018-09-20 12:39:31 -05:00

'...the browser cache'

_'...the browser cache'_

robinnorth (Migrated from github.com) commented

2018-09-20 12:40:16 -05:00

'...like any other page...'

_'...like any other page...'_

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

				```

robinnorth (Migrated from github.com) commented

2018-09-20 12:41:37 -05:00

'...(such as by using JSON.parse()).'

_'...(such as by using `JSON.parse()`).'_

README.md

						
				@@ -568,20 +614,16 @@ document.addEventListener("pjax:success", whenDOMReady)

				_Note: Don't create the Pjax instance in the `whenDOMReady` function._

robinnorth (Migrated from github.com) commented

2018-09-20 12:44:46 -05:00

I'm not sure that this or the line below is useful, so I would be inclined to remove them.

robinnorth (Migrated from github.com) commented

2018-09-20 12:45:31 -05:00

'If you want to just update a specific part of the page (which is a good idea)...'

_'If you want to just update a specific part of the page (which is a good idea)...'_

README.md

						
				@@ -638,2 +678,3 @@

				    // see http://help.disqus.com/customer/portal/articles/472107-using-disqus-on-ajax-sites

				    // if disqus <script> is already loaded, we just reset disqus the right way

				    // see https://help.disqus.com/developer/using-disqus-on-ajax-sites

				    else {

robinnorth (Migrated from github.com) commented

2018-09-20 12:46:50 -05:00

Update URL to https://help.disqus.com/developer/using-disqus-on-ajax-sites

thewatts commented

2018-09-20 13:28:30 -05:00

(Migrated from github.com)

Hey @robinnorth - thanks for the feedback!

I'm going through your suggestions and will add another commit with the fixes :)

Hey @robinnorth - thanks for the feedback! I'm going through your suggestions and will add another commit with the fixes :)

thewatts (Migrated from github.com) reviewed 2018-09-20 13:31:20 -05:00

README.md

						
				@@ -402,7 +463,7 @@ To give context to this CSS, here is an HTML snippet:

				<!doctype html>

thewatts (Migrated from github.com) commented

2018-09-20 13:31:20 -05:00

@robinnorth instead of ...like any other page... here I reversed the language of the true example: ...without a full page reload....

Does that sound okay?

@robinnorth instead of `...like any other page...` here I reversed the language of the `true` example: `...without a full page reload...`. Does that sound okay?

thewatts commented

2018-09-20 13:38:29 -05:00

(Migrated from github.com)

@robinnorth - feedback commit added :)

robinnorth (Migrated from github.com) approved these changes 2018-09-20 14:44:07 -05:00

robinnorth (Migrated from github.com) left a comment

These all look good to me, thanks very much! 👍

thewatts commented

2018-09-20 14:45:29 -05:00

(Migrated from github.com)

@robinnorth Thank You for looking at everything, and for your work on this project. I'm very much enjoying working with it!

@robinnorth _Thank You_ for looking at everything, and for your work on this project. I'm very much enjoying working with it!

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:11:46 -05:00

README.md

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:11:46 -05:00

Is there a reason to change all the bulleted lists from - to *? They're rendered the same anyway.

Is there a reason to change all the bulleted lists from `-` to `*`? They're rendered the same anyway.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:21:58 -05:00

README.md

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:21:58 -05:00

Please remove the bullets from the second level.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:24:09 -05:00

README.md

						
				@@ -313,18 +373,18 @@ var pjax = new Pjax({

				  switchesOptions: {

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:24:08 -05:00

This change is incorrect. These classes are added to the old element that is being replaced, e.g. a fade out.

thewatts (Migrated from github.com) reviewed 2018-09-20 15:24:27 -05:00

README.md

thewatts (Migrated from github.com) commented

2018-09-20 15:24:27 -05:00

@BehindTheMath the original document had both - and * for bullets, so I just converted one to the other.

I have no preference on * over the other.

I can change them all to - if you'd prefer :)

@BehindTheMath the original document had both `-` and `*` for bullets, so I just converted one to the other. I have no preference on `*` over the other. I can change them all to `-` if you'd prefer :)

thewatts (Migrated from github.com) reviewed 2018-09-20 15:25:27 -05:00

README.md

						
				@@ -313,18 +373,18 @@ var pjax = new Pjax({

				  switchesOptions: {

thewatts (Migrated from github.com) commented

2018-09-20 15:25:27 -05:00

@BehindTheMath good catch! This is something I was confused on - I'll adjust!

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:28:34 -05:00

README.md

						
				@@ -313,18 +373,18 @@ var pjax = new Pjax({

				  switchesOptions: {

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:28:34 -05:00

This change makes it less clear. These classes are added to the new element that is replacing the old one, e.g. a fade in.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:31:04 -05:00

README.md

						
				@@ -402,7 +463,7 @@ To give context to this CSS, here is an HTML snippet:

				<!doctype html>

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:31:03 -05:00

Or ...will attempt....

Or *...will attempt...*.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:33:30 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:33:30 -05:00

Please remove the bullet from the 2nd level.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:35:26 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:35:26 -05:00

IMO the old way looked better.

@robinnorth ?

IMO the old way looked better. @robinnorth ?

robinnorth (Migrated from github.com) reviewed 2018-09-20 15:38:50 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

robinnorth (Migrated from github.com) commented

2018-09-20 15:38:50 -05:00

Certainly we should settle on either Title Case or Sentence case throughout, 'about' should be capitalised here if we keep title case.

I usually have a preference for sentence case, FWIW.

Certainly we should settle on either _Title Case_ or _Sentence case_ throughout, 'about' should be capitalised here if we keep title case. I usually have a preference for sentence case, FWIW.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:40:07 -05:00

README.md

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:40:07 -05:00

the original document had both - and * for bullets, so I just converted one to the other.

Ok, I didn't realize that.

I'm not sure which one is more intuitive.

@robinnorth ?

>the original document had both - and * for bullets, so I just converted one to the other. Ok, I didn't realize that. I'm not sure which one is more intuitive. @robinnorth ?

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 15:43:48 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

BehindTheMath (Migrated from github.com) commented

2018-09-20 15:43:48 -05:00

I usually have a preference for sentence case, FWIW.

I do as well (unless it's a proper noun). There are a few places where that was changed.

Truthfully, here I was focusing more on the A.

>I usually have a preference for sentence case, FWIW. I do as well (unless it's a proper noun). There are a few places where that was changed. Truthfully, here I was focusing more on the `A`.

thewatts (Migrated from github.com) reviewed 2018-09-20 15:59:25 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

thewatts (Migrated from github.com) commented

2018-09-20 15:59:25 -05:00

@BehindTheMath would you prefer it all to fall under a single bullet?

ex:

* `X-PJAX-Selectors`: A serialized JSON array of selectors, taken from `options.selectors`. You can use this to send back only the elements that Pjax will use to switch, instead of sending the whole page. Note that you'll need to deserialize this on the server (Such as by using `JSON.parse()`)

@BehindTheMath would you prefer it all to fall under a single bullet? ex: ```md * `X-PJAX-Selectors`: A serialized JSON array of selectors, taken from `options.selectors`. You can use this to send back only the elements that Pjax will use to switch, instead of sending the whole page. Note that you'll need to deserialize this on the server (Such as by using `JSON.parse()`) ```

robinnorth (Migrated from github.com) reviewed 2018-09-20 18:09:30 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

robinnorth (Migrated from github.com) commented

2018-09-20 18:09:30 -05:00

Ah, I see. I think either is OK, it could also just be shortened to 'DOM ready state'. I leave it up to you.

Having re-read it in situ, it seems that it should be a level 2 heading so that it does not appear to be a sub-heading of the preceding HTTP headers section.

Ah, I see. I think either is OK, it could also just be shortened to _'DOM ready state'_. I leave it up to you. Having re-read it in situ, it seems that it should be a level 2 heading so that it does not appear to be a sub-heading of the preceding _HTTP headers_ section.

robinnorth (Migrated from github.com) reviewed 2018-09-20 18:10:27 -05:00

README.md

robinnorth (Migrated from github.com) commented

2018-09-20 18:10:27 -05:00

I normally use - in Markdown lists, so that would be my preference.

I normally use `-` in Markdown lists, so that would be my preference.

BehindTheMath (Migrated from github.com) reviewed 2018-09-20 20:09:37 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

BehindTheMath (Migrated from github.com) commented

2018-09-20 20:09:37 -05:00

You can move the paragraph to the next line, just don't use a bullet. Bullets are for lists, but here there's only 1.

* `X-PJAX-Selectors`
  A serialized JSON array of selectors, taken from `options.selectors`.
  You can use this to send back only the elements that Pjax will use to switch, instead of sending the whole page. Note that you'll need to deserialize this on the server (Such as by using `JSON.parse()`)

You can move the paragraph to the next line, just don't use a bullet. Bullets are for lists, but here there's only 1. ``` * `X-PJAX-Selectors` A serialized JSON array of selectors, taken from `options.selectors`. You can use this to send back only the elements that Pjax will use to switch, instead of sending the whole page. Note that you'll need to deserialize this on the server (Such as by using `JSON.parse()`) ```

thewatts (Migrated from github.com) reviewed 2018-09-21 16:09:52 -05:00

README.md

thewatts (Migrated from github.com) commented

2018-09-21 16:09:52 -05:00

I changed the * to be -

I changed the `*` to be `-`

thewatts (Migrated from github.com) reviewed 2018-09-21 16:10:05 -05:00

README.md

thewatts (Migrated from github.com) commented

2018-09-21 16:10:04 -05:00

Fixed!

thewatts (Migrated from github.com) reviewed 2018-09-21 16:10:24 -05:00

README.md

						
				@@ -313,18 +373,18 @@ var pjax = new Pjax({

				  switchesOptions: {

thewatts (Migrated from github.com) commented

2018-09-21 16:10:24 -05:00

Fixed here - used your terminology provided in your feedback

thewatts (Migrated from github.com) reviewed 2018-09-21 16:10:31 -05:00

README.md

						
				@@ -313,18 +373,18 @@ var pjax = new Pjax({

				  switchesOptions: {

thewatts (Migrated from github.com) commented

2018-09-21 16:10:31 -05:00

Same as above -

thewatts (Migrated from github.com) reviewed 2018-09-21 16:10:48 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

thewatts (Migrated from github.com) commented

2018-09-21 16:10:48 -05:00

Fixed

thewatts (Migrated from github.com) reviewed 2018-09-21 16:11:03 -05:00

README.md

						
				@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)

				document.addEventListener('pjax:complete', topbar.hide)

thewatts (Migrated from github.com) commented

2018-09-21 16:11:03 -05:00

Changed to ## Dom Ready State

Changed to `## Dom Ready State`

BehindTheMath (Migrated from github.com) reviewed 2018-09-21 16:57:28 -05:00

README.md

BehindTheMath (Migrated from github.com) commented

2018-09-21 16:57:28 -05:00

i.e. should be e.g., and the same below.

i.e. = in other words.
e.g. = an example.

`i.e.` should be `e.g.`, and the same below. `i.e.` = in other words. `e.g.` = an example.

thewatts (Migrated from github.com) reviewed 2018-09-21 16:58:21 -05:00

README.md

thewatts (Migrated from github.com) commented

2018-09-21 16:58:21 -05:00

@BehindTheMath will do!

BehindTheMath commented

2018-09-21 16:58:34 -05:00

(Migrated from github.com)

@thewatts Thank you for making those changes.

There are a few remaining comments. Can you take a look at those?

@thewatts Thank you for making those changes. There are a few remaining comments. Can you take a look at those?

thewatts (Migrated from github.com) reviewed 2018-09-21 17:00:50 -05:00

README.md

thewatts (Migrated from github.com) commented

2018-09-21 17:00:50 -05:00

@BehindTheMath fixed :)

thewatts commented

2018-09-21 17:02:13 -05:00

(Migrated from github.com)

@BehindTheMath - I've made those adjustments, and rebased the commit.

Let me know if there's anything else you need!

@BehindTheMath - I've made those adjustments, and rebased the commit. Let me know if there's anything else you need!

BehindTheMath commented

2018-09-22 20:06:01 -05:00

(Migrated from github.com)

There are a few more comments (here, here, here, and here).

There are a few more comments ([here](https://github.com/MoOx/pjax/pull/176/files#r219254546), [here](https://github.com/MoOx/pjax/pull/176/files#r219254759), [here](https://github.com/MoOx/pjax/pull/176/files#r219255132), and [here](https://github.com/MoOx/pjax/pull/176/files#r219255243)).

thewatts commented

2018-09-22 20:10:56 -05:00

(Migrated from github.com)

@BehindTheMath oh sorry I missed those! I never received email notifications of the additional requests.

Working on them right now -

@BehindTheMath oh sorry I missed those! I never received email notifications of the additional requests. Working on them right now -

thewatts commented

2018-09-22 20:13:48 -05:00

(Migrated from github.com)

@BehindTheMath all set!

👍 1

BehindTheMath (Migrated from github.com) approved these changes 2018-09-22 20:20:28 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#176

				`@@ -402,7 +463,7 @@ To give context to this CSS, here is an HTML snippet:`
				`<!doctype html>`

				`@@ -568,20 +614,16 @@ document.addEventListener("pjax:success", whenDOMReady)`

				_Note: Don't create the Pjax instance in the `whenDOMReady` function._

				`@@ -313,18 +373,18 @@ var pjax = new Pjax({`
				`switchesOptions: {`

				`@@ -518,41 +573,32 @@ document.addEventListener('pjax:send', topbar.show)`
				`document.addEventListener('pjax:complete', topbar.hide)`