-
Notifications
You must be signed in to change notification settings - Fork 62
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: collect user feedback #965
Conversation
Closes: #943
I appreciate not wanting to push our own styling, but the mixture of serif (most text) and sans-serif (buttons) doesn't look great, and Times New Roman is meh. :) How about something like this (from CSS tricks): #honeybadger-feedback {
font-family: system-ui, "Segoe UI", Roboto, Helvetica, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol";
} |
Awesome, thanks! |
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.
Cool! I had a few questions.
Also what's the best way for me to try this out? Use the React example project?
…or testing locally Refs: #943
Thanks for the review, I applied all your feedback! I also committed my changes in the example apps to try this out locally:
With a couple of tweaks, I think you should be able to use the react example app instead of the webpack one. |
I tried following these instructions, and while it's setting |
# Conflicts: # packages/react/package-lock.json
@joshuap That's because I left out the most important part 🤦 . You will need to call Here are a few screenshots on what you should expect in the console and network tabs of dev tools: |
@subzero10 I pulled your branch and did The reason I even ran the build command is because I followed your test instructions and didn't get the error reported correctly, and I noticed that
|
@BethanyBerkowitz I had this happen to me a couple of times. I resolved it by removing all This happens because sometimes the internal packages that are dependencies (i.e. @honeybadger-io/core) are not properly linked in |
@joshuap, @BethanyBerkowitz and I just realized another thing. Probably you already know about it, but pointing it out here just in case:
|
It worked for me too! 👌 |
I think all feedback has been resolved. |
@subzero10 do you want to switch to the |
I was planning to do it in a separate PR, but I tried on this one since you suggested :) And... It's not working for some reason. Is the change deployed on the backend? I'm getting a 301 Redirect and then a 403. I looked at my project and verified that the comment is not added to the notice. Note: I'm issuing a |
Ah, it is not deployed — I added the new endpoint to the wrong repo. :) I have a PR ready, so it should be deployed soon. |
Ok, now you should be good for testing |
cf11503
to
a58440a
Compare
a58408e
to
3ebd91e
Compare
Update with the latest commits:
@stympy This is ready for final review. I will merge with your approval. Thanks! |
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.
Status
READY
Description
Closes: #943
Todos