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

[INS-1517] adds GitHubStarsButton #5009

Merged
merged 9 commits into from
Jul 27, 2022

Conversation

dimitropoulos
Copy link
Contributor

@dimitropoulos dimitropoulos commented Jul 26, 2022

This PR adds a GitHub star tracker, as commonly seen in many applications.

Some notes

  • if incognito mode is enabled, it will not make any network requests
  • app-wide unnecessary rerendering influenced the design:
    • when using useEffect the component was benchmarked to rerender 8 times (despite no props changes)
    • when using useMount it is improved (as, it only fetches on mount) to when changing insomnia activities (broadly speaking)

App Dashboard

BEFORE AFTER
Screenshot_20220726_192216 Screenshot_20220726_191908

Request Collection

BEFORE AFTER
Screenshot_20220726_192705 Screenshot_20220726_192634

Design Document

BEFORE AFTER
Screenshot_20220726_192234 Screenshot_20220726_192744

Unit Test

BEFORE AFTER
Screenshot_20220726_192322 Screenshot_20220726_191938

Incognito Mode

Also works well in themes

state
neutral state Screenshot_20220726_192519
hover "Star" Screenshot_20220726_192456
hover count Screenshot_20220726_192433

Segment

As with the rest of the application, segment events will not fire if "send usage statistics" is false (including by being set by incognito mode). If true, here is what the events look like (cc: @wdawson):

changelog(Improvements): Adds a GitHub stargazer button to the app header

@dimitropoulos dimitropoulos requested a review from marckong July 26, 2022 17:27
@marckong
Copy link
Contributor

I did run through some smoke test. I noticed two things in the case of offline;

  1. count not rendering (makes sense since its offline, but in this case should we show the button at all?)

image

2. there are some weird React flush errors when it's offline. I suppose it may be coming from the fetch logic. I am not sure if this is a big deal since it does not occur in the online state.

image

Otherwise, this PR looks good to me and the description of the PR is detained and amazing!

@marckong marckong requested a review from a team July 26, 2022 18:32
@marckong
Copy link
Contributor

I left minor comments on something trivial and this PR looks good to me. I ran some smoke tests as well and it looks working as expected. One edge case may be handling the offline status, but I haven't been able to reproduce it after first few times of reload.

Copy link
Contributor

@wdawson wdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

I would prefer to keep the github url intact so make it easier to search for should we ever need to update it, but non-blocking and kinda opinionated.

Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

LGTM!

⚠️ I found 2 weird behaviors, but I leave it up to you to see if they are worth investigating or fixing in this PR:

  • There's a slight discrepancy in stargazers numbers when you disable Incognito. It's only noticeable up to the point you switch to another tab. For example, first we show 21700 stargazers, then upon switching to another Design Document tab, we re-render with a new number of 21749

Screen Shot 2022-07-27 at 10 41 14

Screen Shot 2022-07-27 at 10 41 20

  • When we work offline / network disabled, the element is a bit jittery. It tries to render the star count, only to be back to just the star button in a split second:
Kapture.2022-07-27.at.10.46.47.mp4

@dimitropoulos
Copy link
Contributor Author

thanks @filfreire for finding that. d07c58f fixes it (and as a note for the future, @gatzjames / @jackkav / @filfreire all saw and approved of the approach in that commit, with the suggestion from @gatzjames that in a future PR we can improve this further by hardcoding the fallback (today, set to 21700) with an environment variable that's fed into the production build by the CI).

@dimitropoulos dimitropoulos merged commit 9df9b9a into Kong:develop Jul 27, 2022
@dimitropoulos dimitropoulos deleted the feat/github-stars branch July 27, 2022 17:25
@dimitropoulos
Copy link
Contributor Author

@jackkav I did 3074a0a in response to your feedback -> thanks!

@DanielRouxSA
Copy link

Ahh I hate to be that person, but is there perhaps a way to hide this control? I find it doesn't meaningfully add to my experience with the app, and I find it visually cluttering.

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.

6 participants