-
-
Notifications
You must be signed in to change notification settings - Fork 724
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
feat: docs feedback form created #609
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
As discussed in the issue, we can have netlify functions to implement the functionality for implementing the process. Right now, data is going through a |
@akshatnema Thanks for that PR. I don't have permissions to create/setup such a envs and then connect them in the Netlify (we need your help @fmvilas @derberg). As I understand you have backend but you didn't push it. Of course you can, it's not a problem. We don't need to wait for envs. However I'm not a fan of having access to that envs in the PRs. Why? People can make simple console log and then log all envs that we have. It's not a good idea. As I know Netlify has possibility to setup envs only for master branch and currently we use that for Zenhub and Github tokens (for fetching the roadmap data). About Netlify function or API on the NextJS side: we use the static site generation, but if that API works with static site (please build site and then host it in your local machine and try run that API) we can go with that solution, it will be perfect :) So please push changes for API and I can make review. |
@magicmatatjahu Yepp, I pushed the backend in the PR. And as expected, the form will not work as there will be an internal server error (shown in the Browser console window). This happens because there is no OAuth and spreadsheet connected to it using envs. API is working perfectly fine in my local environment as you can see in the preview above, but I can't able to deploy it on my Netlify account. Errors are already sent to you on Slack. I also want to raise certain issues on design perspective and want to discuss with you:
|
Please don't worry, as I wrote, we should not add access to these envs on the PRs, because people will be able to log them. They should be only available on master build. Also I see that your PR was properly built, so is there a problem yet?
You should retrieve name of doc from NextJS Router Context (I guess, you should test it) :)
I checked some sites (including NextJS documentation) and I see that feedback form is always after the next/prev buttons so I think the current location is good :) In addition, we could display only the "add feedback" button at the beginning and when someone clicks it, the whole form will appear. Currently it is very "explicit" (I don't know how to describe it 😅 ) in the documentation and people can focus on it and not on the documentation. |
@magicmatatjahu Ok 👍, will definitely look into this and will update the backend accordingly.
Ok, will look into this and I will try to implement it in a better way. As it was not mentioned before in the issue, I haven't thought of it yet. |
@akshatnema token on Netlify that has rights to create discussions is there, env name -> |
@derberg Thanks Lukasz for creating it 😊. It will be needed when this is pushed in production. But, I need some more details regarding createDiscussion functionality. Here are they:
I will also like a review of @magicmatatjahu on the above questions mentioned. |
But I have to say I do not really have a strong opinion on above 🤷🏼 except for that I think it must be website repo |
@derberg the approach you mentioned is even more hard to implement because if you see the implementation and rendering of the component, you will find that on each doc, the same component is rendered without any specific components for each doc. This means that the feedback submitted on a doc, the response we get back can be seen from every doc 🙂. So, that's what I am planning to have either NextJS Context or some variable to uniquely identify the feedback for each doc. We can surely can't make comments approach (as mentioned by you) because there are no different routes defined for the POST request made in the component. Hence, I will suggest to have createDiscussion functionality for the feedback. |
Feedback on discussion category URLHey, @derberg and @akshatnema! 😄 Just a comment on the created I already have a Question on SUBMIT button in feedback cardI used the Netlify preview link to test how the feedback card is working so far. I see that the |
Fine, I will add the category and repository ID to that one in mutation. It will then create the discussion on that particular category 🙂.
@alequetzalli Regarding the working of this button, yes it is not working right now because I pushed the backend of the form, using Google Sheets API, but due to the unavailability of environment variables, it will not work right now. I recommend you to read the description of this message #609 (comment). See the preview of the button (as shown in the video) and you will get what it does. |
Hey @magicmatatjahu @derberg @alequetzalli, I have made the backend functionality of creating a GitHub discussion on Preview of the Functionality:GitHub.Discussions.mp4Keypoints to notice
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akshatnema WOW, thank you for all of your work!!!!! 🎉🎉❤️✨✨
There are just some English typos/grammar fixes, and then we're done from the TW side. ✌🏽
Co-authored-by: Alejandra Quetzalli <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we got this 🚀
@alequetzalli and @magicmatatjahu also must approve as they requested changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢🚢🚢🌈🌈✨✨✨✨✨
@derberg any updates for this PR? |
As everyone who was supposed to give approve gave approve, I merge. Let's hope there are no bugs in production 😄 |
/rtm |
in the console I get 401 error but I checked and token is there in Netlify, with the name as you have it in code 🤔 |
@derberg Could you check logs of the Netlify Function? I don't have permission, but you should. We print error to the console, so there should be more info. |
@derberg @magicmatatjahu So sorry, I found the error immediately. I forgot to change the category and repo ID after testing. I checked it right now and will soon correct it with the actual ones. Secondly, I got that in error view, |
@akshatnema It is still failing after redeploy of the fix 😅
|
@akshatnema we only need a token of |
@derberg Nope, it requires more than that. More specifically, it is kind of GitHub OAuth added to the backend which approves that the user is verified and then allows creating a Discussion. Hence, you need to give more permissions to the token. Following image will describe which permissions you need to give. You can probably not give |
ok, not a token problem, I recreated it, with all possible scopes, like a god mode, and still |
|
The issue is only related to the authentication (since it is giving the error of 401). You can try to deploy the website again by using the feature If you couldn't, let's catch up someday in a Zoom meeting where we can surely check it out from the logs what's going wrong in production. I'm not free this week probably because I'm having my exams so you can arrange a meeting on 29th April around 6PM (IST) or in the next week. I'm even free on weekend if you are suitable with 😅. |
I checked token with my postman client to call GraphQL API in the way you do it in your code and it worked fine. This is not needed but still, it should not cause 401 |
It hasn't created any error in the local environment yet. I'm not sure if this can create 401 error at production. |
oh, I was actually now aware redeployment is needed after variable value change....Martin Fowler would not be happy about it 😆 Lemme try |
@derberg We did it 😄. But please delete this discussion so that Alejandra doesn't take it as feedback given on docs 😃. |
congrats @akshatnema on amazing implementation 👏🏼 |
Thanks @derberg. I think you should now create an issue in the website repo on how to improve the title of the Discussion created by feedback. It is really looking very awkward right now for me. |
yeah, let's see how people use it first, how often, etc |
Description
This PR is related to issue #453 where I have added a Docs Feedback Form at the end of each doc to take feedback from the users and store it in a Google spreadsheet connected through Google OAuth and Google Sheets API. So far discussed in the issue, I have implemented the design in the PR and it is working fine. Also, the backend facility of taking the data to the spreadsheet has been made and it is in my local dev environment. It is because some Google environment variables are not set in the netlify deployment, so the facility will fail on the deployment as soon as it is pushed. I request the maintainers and community founders to kindly add more environment variables in the production to make this feature fully responsive on the website.
Here's the list of env variables to be added:
Apart from the above variables, we need to create Google Service Account (if hasn't been created yet) and enable the Editor role to that account in the spreadsheet. I will highly recommend if @magicmatatjahu or @fmvilas could connect to me in a call for further explanations of what exactly will be needed to set up the variables. This PR will be as a draft PR till the environment variables and the backend will be pushed in the repository. Any suggestions are warmly welcomed on this PR 😄.
Preview of the backend facility from my local dev environment
2022-02-28.12-33-43.mp4
Related issue(s)
Resolves #453