Skip to content

Added new blog for a new frontend plugin and its integration with backend plugin #233

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

Conversation

sgahlot
Copy link
Contributor

@sgahlot sgahlot commented Oct 13, 2023

What does this PR do / why we need it

This PR contains a new blog detailing how to create a new frontend plugin and integrate it with a backend plugin

Which issue(s) does this PR fix

Fixes #?

PR acceptance criteria

  • Unit Tests
  • E2E Tests
  • Documentation

How to test changes / Special notes to the reviewer

Review the new blog by visiting blog/2023/10/11/frontend-plugin

@sgahlot sgahlot requested a review from a team as a code owner October 13, 2023 20:43
@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 13, 2023

This is my first PR. Please let me know if I need to add anything or if I missed something. I have gotten this blog reviewed by my team, so the code/contents of the blog are technically correct but I'll welcome any feedback in case there is still something lacking and needs improvement.

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

@Zaperex I've updated the blog code based on your feedback. Can you please review it and let me know if there are still issues?
Please also let me know if I should squash all the commits in this PR into a single one.

@Zaperex
Copy link
Member

Zaperex commented Oct 20, 2023

@sgahlot I think you missed some of the suggestions.
image

Github probably collapsed those suggestions due to the number of suggestions I made.

Also you won't need to squash the commits, since we do that when we merge the PR.

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

Ah. My bad. Let me look at those as well. Thanks for pointing that out :-)

@Zaperex
Copy link
Member

Zaperex commented Oct 20, 2023

Also no need to ping me for every single suggestion 😄, you can just reply without the ping or just make the changes in the commit and not reply to the suggestion.

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

Sorry about too many pings. Will avoid it in future

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

Fixed for remaining comments

Comment on lines 74 to 76
#### Modify `package.json`

Modify `package.json` in the root directory by adding the following content in the `scripts` section (_just after the `start` script entry_):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#### Modify `package.json`
Modify `package.json` in the root directory by adding the following content in the `scripts` section (_just after the `start` script entry_):
#### Modifying the `package.json`
Modify the `package.json` in the root directory by adding the following content in the `scripts` section (_just after the `start` script entry_):

@Zaperex
Copy link
Member

Zaperex commented Oct 20, 2023

@sgahlot Just 2 more small changes that I missed from the previous review, other than that it looks good to me.

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

@Zaperex Updated for the 2 changes you recommended.

Copy link
Member

@Zaperex Zaperex left a comment

Choose a reason for hiding this comment

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

@sgahlot Just found a few more changes regarding wording. Sorry for not catching these in the initial review. You don't need to ping me for each suggestion, just make a commit with the changes and I'll check it afterwards.

@Zaperex
Copy link
Member

Zaperex commented Oct 20, 2023

@sgahlot you missed a few suggestions again. Github collapsed them again because of the number of suggestions
image

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

Ah man. I must be going blind :-). Sorry about that. Let me try again

Copy link
Member

@Zaperex Zaperex left a comment

Choose a reason for hiding this comment

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

Just 2 last changes, I can merge them for you if you want. Otherwise, LGTM

@Zaperex Zaperex requested a review from schultzp2020 October 20, 2023 19:22
@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

Let me make these changes as well and commit

Copy link
Member

@schultzp2020 schultzp2020 left a comment

Choose a reason for hiding this comment

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

LGTM

@Zaperex
Copy link
Member

Zaperex commented Oct 20, 2023

@sgahlot Let me know if you still want to make the changes yourself, otherwise I'll merge the PR.

@sgahlot
Copy link
Contributor Author

sgahlot commented Oct 20, 2023

@Zaperex sure, please go ahead with the merging of this PR. Much appreciated.

@Zaperex Zaperex merged commit 532c9ce into janus-idp:main Oct 20, 2023
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