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

ul blog improvements #1522

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

ul blog improvements #1522

wants to merge 2 commits into from

Conversation

isemona
Copy link
Contributor

@isemona isemona commented Aug 18, 2024

This blog post has the following features:

  • A GitHub Repository with a polished README
  • A GitHub Repository under the github.com/oktadev account
  • A title that's approved by Dev Advocacy
  • A URL approved by Dev Advocacy
  • The content has been run through Grammarly (https://www.grammarly.com/)
  • Rendered locally and confirmed that no Markdown typos exist
  • Images are compressed appropriately
  • Social image previews well on Twitter and LinkedIn
  • Tech review request (developer advocate or domain expert)
  • Review request for editorial/grammar/clarity (developer advocate or Colton)

Copy link

netlify bot commented Aug 18, 2024

Deploy Preview for okta-blog ready!

Name Link
🔨 Latest commit 2d9326e
🔍 Latest deploy log https://app.netlify.com/sites/okta-blog/deploys/66c2b4a2642b6d0008a2e437
😎 Deploy Preview https://deploy-preview-1522--okta-blog.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@edunham edunham left a comment

Choose a reason for hiding this comment

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

👍 from a "doesn't break anything I have insight into" perspective. This is not a technical review of the new code you've added.

Copy link
Member

@alisaduncan alisaduncan left a comment

Choose a reason for hiding this comment

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

changes requested

email: email
},
});
return res.sendStatus(httpStatus);
Copy link
Member

Choose a reason for hiding this comment

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

?? am i missing a paren indention? This looks like you are returning out of the method first, then there's code that will never get hit to check if the user doesn't exist. I don't understand. is the intent for the 404 check to move higher?

});

universalLogoutRoute.use((err,req,res,next) => {
if(err){
Copy link
Member

Choose a reason for hiding this comment

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

What's an error and why are they all 404?

@@ -602,7 +668,7 @@ if (!res.ok)
}}
```

The onNewTask function will now look like this:
The onNewTask function will now look like this with a change made only to the `onNewTAsk` function:
Copy link
Member

Choose a reason for hiding this comment

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

You should re-evaluate this sentence. It seems repetitive. Also change casing for onNewTAsk

// Redirect user back to the sign in page
window.location.href = '/';
// Redirect user back to the sign in page
window.location.href = '/';
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, why are using window API here to redirect to home. I thought this is handled already inside the todo app, can't we use React Router?

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.

3 participants