Skip to content

Comments

[EDU-765] Update jwt authentication tutorial#1502

Merged
tbedford merged 32 commits intomainfrom
update-jwt-authentication-tutorial
Aug 10, 2022
Merged

[EDU-765] Update jwt authentication tutorial#1502
tbedford merged 32 commits intomainfrom
update-jwt-authentication-tutorial

Conversation

@fliptopbox
Copy link
Contributor

@fliptopbox fliptopbox commented Jul 25, 2022

Description

As part of the stocktaking of all tutorials (see: https://ably.atlassian.net/browse/EDU-765)
This change repated to the updated tutorial PR ably/tutorials#104
we found inconsistencies, and failing code samples, for the most visited tutorial.

This PR is a QUICK-FIX the following:

  • the Textile instructions do not match the Github repo code snippet (see: https://github.com/ably/tutorials/tree/jwt-authentication-nodejs-fix) that fixed this mismatch
  • the input form on the demo is not used to set the clientId and is redundant, the fix corrects this redundancy
  • the JavaScript ES6 updates and styling changes for: use of const, and spread operator, and fat arrow functions.

Related work:

  • Update the live demo CSS for docs on Website page Website PR
  • Update the docs tutorial, to use the update screenshot and updated JavaScript docs tutorial PR
  • Update the tutorials source code, and replace out-of-date ably logo. Source code PR

@fliptopbox fliptopbox marked this pull request as draft July 25, 2022 11:29
@fliptopbox fliptopbox self-assigned this Jul 25, 2022
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 25, 2022 11:29 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 25, 2022 16:03 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 25, 2022 16:06 Inactive
@fliptopbox fliptopbox requested review from marklewin and tbedford July 25, 2022 16:08
@fliptopbox fliptopbox marked this pull request as ready for review July 25, 2022 16:08
@fliptopbox
Copy link
Contributor Author

@tbedford & @marklewin this is my very first docs contribution, so i am tending on the side of caution for this PR.
Please note this has a complimentory PR on the docs repo. See ably/tutorials#104

@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 25, 2022 16:18 Inactive
@kennethkalmer kennethkalmer had a problem deploying to ably-docs-update-jwt-au-sgbiph July 25, 2022 16:26 Failure
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 26, 2022 08:23 Inactive
Copy link
Contributor

@marklewin marklewin left a comment

Choose a reason for hiding this comment

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

Looks good. Love the fancy new dialog 👏 I've made a few suggestions to help make the instructions a bit clearer and fix a couple of grammar/typo things.

4. Navigate your browser to @http://localhost:3000@, since your Express server is listening on port 3000.

You will see the login form. Open the browser JavaScript console if you want to see the logged messages.
You will see the login form. Open the browser JavaScript console if you want to see the logged messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest making the opening of the browser console a step in its own right, otherwise there's not really any point in having the console.log() statements.

@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 28, 2022 10:16 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 28, 2022 10:17 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 28, 2022 10:17 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 28, 2022 10:19 Inactive
@fliptopbox fliptopbox force-pushed the update-jwt-authentication-tutorial branch from 850f045 to a63fb96 Compare July 28, 2022 11:33
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 28, 2022 11:33 Inactive
@fliptopbox fliptopbox requested a review from marklewin July 28, 2022 11:33
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 28, 2022 16:32 Inactive
Copy link
Contributor

@marklewin marklewin left a comment

Choose a reason for hiding this comment

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

Great start. A few typos and rewording suggestions and also for improving the live demo.

@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 29, 2022 09:31 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 29, 2022 09:32 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 29, 2022 09:38 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 29, 2022 10:25 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 29, 2022 10:46 Inactive
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph July 29, 2022 10:47 Inactive
@fliptopbox fliptopbox requested a review from marklewin July 29, 2022 10:48
fliptopbox and others added 24 commits August 10, 2022 14:56
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
Co-authored-by: Mark Lewin <mark.lewin@ably.com>
@fliptopbox fliptopbox force-pushed the update-jwt-authentication-tutorial branch from 0ecf59c to 52dde6f Compare August 10, 2022 13:57
@kennethkalmer kennethkalmer temporarily deployed to ably-docs-update-jwt-au-sgbiph August 10, 2022 13:57 Inactive
@fliptopbox fliptopbox requested a review from tbedford August 10, 2022 14:00
Copy link
Contributor

@tbedford tbedford left a comment

Choose a reason for hiding this comment

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

I've run through and tested. LGTM!

@tbedford tbedford merged commit c6567dc into main Aug 10, 2022
@tbedford tbedford deleted the update-jwt-authentication-tutorial branch August 10, 2022 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Do not merge When the PR is in progress do not merge

Development

Successfully merging this pull request may close these issues.

4 participants