-
Notifications
You must be signed in to change notification settings - Fork 78
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: CTA Button to Star Github Repo #532
Conversation
@jayantbh @e-for-eshaan We have 2 good options to show this, either in sidebar or in header of a page. Is there any plan to add something on right side of header? |
We want it to be frictionless to be able to star the repo. Putting it in the side bar is basically 'hiding it' |
It's difficult to infer from this that these are stars, there needs to be some sort of star icon or something similiar for people to know that they can star the repo by clicking this |
@e-for-eshaan Unauthorized Github API calls are limited to 60 per hour. We can use fetch() in GithubButton component directly to utilize browser caching which has 60 seconds of cache invalidation time. This will almost never exceed rate limit. Any suggestions on this approach? |
Why is rate limits a concern? 😄 It can just be loaded once on app load and that's it. |
@jayantbh So currently I used NextJs api to fetch stars. For every user, the client will call the Github API from BFF. So you mean we call Github API from client side and store count in redux? |
Yeah that's probably best. While traffic is expected to be low on individual instances of Middleware, in case someone hosts it internally widely enough it'll totally break. But the browser making the request is just super unlikely to break unless they are trying to break it. |
@Kamlesh72 how's this turning out? Let us know if you need any help, suggestions and/or reviews. |
@e-for-eshaan I was busy in some urgent work past 10 days, now free 😃. Made the required changes. which extension? Comment Ref |
not really an extension, drew it myself |
Hey @Kamlesh72 can we have some updated images and videos on the PR description |
@e-for-eshaan done |
<StarBorderOutlined fontSize="small" /> | ||
<Typography fontWeight="bold" marginLeft={1}> | ||
Star | ||
</Typography> |
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.
Wrap in a FlexBox
components. Use alignCenter
prop to fix the jagged alignment.
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 hate to request a change as tiny as this, but this is the last mile. This PR is almost done! |
@jayantbh @e-for-eshaan Any update on this PR? Am I missing something? |
Approved! |
Congrats on the contribution sire, took a while but good work! |
Linked Issue(s)
Fixes #454
Acceptance Criteria fulfillment
Proposed changes