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

feat: repository selection & add to insights #693

Merged
merged 22 commits into from
Jan 3, 2023
Merged

Conversation

nightknighto
Copy link
Contributor

@nightknighto nightknighto commented Dec 20, 2022

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

  • Feature: ability to select 1 or more repositories from the Repository tab.

  • Selected Repositories can be used as items for a new insights page for the user.

  • Feature: clicking on an already filtered-by repo will remove the filter

  • Unsigned users clicking on "Add to Insight" will be lead to login, then after logging in will be redirected to insights/hub with the selected repos.

Implementation: Since logging in always leads to homepage and does a page refresh, we will use session storage to save that we want to redirect to a certain link and also save the selected repos, then in homepage we will check if there's a saved redirect link. If there is, we will redirect to it instead.

Related Tickets & Documents

fixes #593
fixes #592

Mobile & Desktop Screenshots/Recordings

image

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Dec 20, 2022

❌ Deploy Preview for oss-insights failed.

Name Link
πŸ”¨ Latest commit cca056e
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/63b083614d4ac20008e3c539

@netlify
Copy link

netlify bot commented Dec 20, 2022

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit cca056e
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/63b08361c4ac150008081fd6
😎 Deploy Preview https://deploy-preview-693--design-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@nightknighto nightknighto marked this pull request as ready for review December 21, 2022 21:36
@nightknighto nightknighto requested a review from bdougie December 21, 2022 21:36
@bdougie
Copy link
Member

bdougie commented Dec 21, 2022

This is an excellent first pass. I have a few questions.

  1. Why is the button green?
  2. This feature should redirect someone to log in at some point. We can't create a page without knowing the user. This should be available for users not logged in. My suggestion is when the user clicks the "Add to Inishgt Page."
  3. What happens when you select more than 10 repos?
  4. The click-to-filter feature was removed in this PR. Related - bug: Click to un-filter by repoΒ #592

click-to-filter

I also found a unrelated bug while QAing this #704

@nightknighto
Copy link
Contributor Author

nightknighto commented Dec 21, 2022

  1. Why is the button green?

that's the default color design of the pre-existing components in the project: checkbox & button. Thought it was intentional for consistency.

  1. This feature should redirect someone to log in at some point. We can't create a page without knowing the user. This should be available for users not logged in. My suggestion is when the user clicks the "Add to Inishgt Page."

Yes sure.

  1. What happens when you select more than 10 repos?

It works for any number of repos. Tested with 20 repos and it works. The implementation isn't affected by the number of repos

I also found a unrelated bug while QAing this #704

I just tested it and it works on the live site.

@bdougie
Copy link
Member

bdougie commented Dec 21, 2022

I also found a unrelated bug while QAing this #704

I just tested it and it works on the live site.

Confirmed this is only an issue on beta

@nightknighto
Copy link
Contributor Author

The click-to-filter feature was removed in this PR. Related - #592

Well that's wierd. It's supposed to automatically merge that with this PR

@bdougie
Copy link
Member

bdougie commented Dec 21, 2022

  1. This feature should redirect someone to log in at some point. We can't create a page without knowing the user. This should be available for users not logged in. My suggestion is when the user clicks the "Add to Inishgt Page."

For clarification. The feature should be useable to select repos, just redirecting the user when they try to navigate to the create page. There may be a small challenge in storing the saved repos after the redirect.

i.e. I choose 3 repos and get redirected when I hit create to login in. After logging in, I should see the repos in the shopping cart on the create page.

@nightknighto
Copy link
Contributor Author

I've noticed some janky/buggy behavior in some cases of selecting repos, so I'll add more fixes

@nightknighto
Copy link
Contributor Author

nightknighto commented Dec 21, 2022

So I've had a look in UseSupabaseAuth hook, and I've found this

return {
    signIn: (data: SignInWithOAuthCredentials) => supabase.auth.signInWithOAuth({
      ...data,
      options: {
        redirectTo: process.env.NEXT_PUBLIC_BASE_URL ?? "/"
      }
    }),

The part

options: {
        redirectTo: process.env.NEXT_PUBLIC_BASE_URL ?? "/"
      }

is basically hard-coding the sign-in to redirect to the root path, even if a custom redirect was passed to it. Is there a reason why it is implemented that way?

@bdougie
Copy link
Member

bdougie commented Dec 21, 2022

@brandonroberts can shed light on it. It came down to the authentication not playing nice with next.

I believe there is context in ClickUp

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

  1. Match the button color to orange
  2. Prevent this feature from showing unless the user is logged in
  3. Ensure this feature still works bug: Click to un-filter by repoΒ #592

@brandonroberts
Copy link
Contributor

@Deadreyo that may have been a side effect of the auth integration we had at the time. You can modify the method signature to take the additional options and apply them to the sign in redirect.

@nightknighto
Copy link
Contributor Author

@Deadreyo that may have been a side effect of the auth integration we had at the time. You can modify the method signature to take the additional options and apply them to the sign in redirect.

I've tried playing around a lot with the redirectTo option but it always failed to work. Even changed the above to

signIn: (data: SignInWithOAuthCredentials) => supabase.auth.signInWithOAuth({
      ...data,
      options: {redirectTo: 'http://localhost:3000/hub/insights'}
    }),

and still leads to /

@bdougie
Copy link
Member

bdougie commented Dec 24, 2022

Would be able to use the api folder and manage this with a serverless function?

https://www.netlify.com/blog/2021/12/11/serverless-functions-made-simple-just-add-files/

@nightknighto
Copy link
Contributor Author

Would be able to use the api folder and manage this with a serverless function?

https://www.netlify.com/blog/2021/12/11/serverless-functions-made-simple-just-add-files/

Hmm, not sure how that can help here. Signing in happens by going to a github page from the frontend so not sure if it's doable to change the signing in method with a serverless function (that's the usecase I thought of related to serverless functions)

@nightknighto
Copy link
Contributor Author

Reverted the redirect commit to unblock this PR. Redirect should be handled in a separate issue

@0-vortex
Copy link
Contributor

  • Unsigned users clicking on "Add to Insight" will be lead to login, then after logging in will be redirected to insights/hub with the selected repos.

I've investigated the issue of logging in random parts of the app and I don't think it is possible to consistently redirect to the second session managed system by just using redirectTo. Similarly to what @Deadreyo was describing as not working, is actually a more complex redirect system that can lose the origin header in multiple scenarios - this makes it impossible to redirect to something other than / without a middleware system accounting for server and client differences of where the user was in the onboarding process or other similar global state remembering where you were.

For this reason, I think we should abort the selection of repos before logging in, hint at the checkbox with a disabled state, and in case the user clicks to log in, would be redirected to the same page (from the middleware), where one could now select the repos to add to the insight page. This invalidates current checks for user and removes a lot of the boilerplate code to check for that in subsequent onChange functions πŸ•

@nightknighto
Copy link
Contributor Author

I think we should abort the selection of repos before logging in, hint at the checkbox with a disabled state

Done.

@nightknighto nightknighto requested review from brandonroberts, 0-vortex and bdougie and removed request for brandonroberts, 0-vortex and bdougie December 30, 2022 18:19
Copy link
Contributor

@0-vortex 0-vortex left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM! πŸ‘

Testing the disabled checkbox now am wondering if some form of extra feedback is needed, like the transition from automatically logging in and being sent to the wrong page versus the checkbox not doing anything while logged, but also not hinting at the user having to log in. Not sure what the best approach is here, let's wait for the triage team to test this out in the UI as well ❀️ πŸ•

Copy link
Contributor

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

LGTM

@brandonroberts
Copy link
Contributor

Maybe some alt text on the checkboxes/button to "Connect to GitHub" would be enough on desktop at least

@nightknighto
Copy link
Contributor Author

Maybe some alt text on the checkboxes/button to "Connect to GitHub" would be enough on desktop at least

Added this
image

@brandonroberts brandonroberts merged commit ba6405d into beta Jan 3, 2023
@brandonroberts brandonroberts deleted the save-to-dashboard branch January 3, 2023 14:28
github-actions bot pushed a commit that referenced this pull request Jan 3, 2023
## [1.23.0-beta.6](v1.23.0-beta.5...v1.23.0-beta.6) (2023-01-03)

### πŸ• Features

* add Edit Page button to insight pages ([#738](#738)) ([bde1eb8](bde1eb8)), closes [#685](#685)
* enable repository selection & add to new insights page ([#693](#693)) ([ba6405d](ba6405d)), closes [#593](#593) [#592](#592)

### πŸ› Bug Fixes

* add missing title props ([cdcc28d](cdcc28d))
github-actions bot pushed a commit that referenced this pull request Jan 5, 2023
## [1.23.0](v1.22.2...v1.23.0) (2023-01-05)

### πŸ§‘β€πŸ’» Code Refactoring

* update pr velocity indicator ([#707](#707)) ([7e96735](7e96735)), closes [#684](#684)

### πŸ› Bug Fixes

* add missing title props ([cdcc28d](cdcc28d))
* correct some tooling and dependency issues ([#749](#749)) ([cb4ec9f](cb4ec9f))
* only pull insights for stacked avatar on insight pages ([#761](#761)) ([80ae119](80ae119))
* overflow x bug on dashboard ([#709](#709)) ([b022dbc](b022dbc)), closes [#677](#677)
* user profile auth avatar processing errors ([#755](#755)) ([3ba87a6](3ba87a6)), closes [#733](#733)

### πŸ• Features

* add Edit Page button to insight pages ([#738](#738)) ([bde1eb8](bde1eb8)), closes [#685](#685)
* add favorite repos on the design system ([#744](#744)) ([c8660b5](c8660b5)), closes [#717](#717)
* add hover card and expand on contributor stack ([#742](#742)) ([9f45b3d](9f45b3d))
* adjust search font-size ([c451450](c451450))
* change name to enhace from logarithmic ([#743](#743)) ([4384324](4384324))
* enable repository selection & add to new insights page ([#693](#693)) ([ba6405d](ba6405d)), closes [#593](#593) [#592](#592)
* Filter dashboard scatter plot by PR states ([#736](#736)) ([f04093a](f04093a))
* handle close modal when click outside and improve select usage ([#705](#705)) ([105a47b](105a47b)), closes [#689](#689)
* update icon PR  details row ([#706](#706)) ([dac42ba](dac42ba)), closes [#696](#696)
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

πŸŽ‰ This PR is included in version 1.23.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

0-vortex added a commit that referenced this pull request Jan 5, 2023
* origin/main: (29 commits)
  chore(minor): release 1.23.0 [skip ci]
  chore(minor): release 1.23.0-beta.13 on beta channel [skip ci]
  feat: update icon PR  details row (#706)
  chore(patch): release 1.23.0-beta.12 on beta channel [skip ci]
  fix: only pull insights for stacked avatar on insight pages (#761)
  chore(patch): release 1.23.0-beta.11 on beta channel [skip ci]
  refactor: update pr velocity indicator (#707)
  chore(minor): release 1.23.0-beta.10 on beta channel [skip ci]
  feat: add hover card and expand on contributor stack (#742)
  chore(patch): release 1.23.0-beta.9 on beta channel [skip ci]
  fix: user profile auth avatar processing errors (#755)
  chore(minor): release 1.23.0-beta.8 on beta channel [skip ci]
  feat: handle close modal when click outside and improve select usage (#705)
  chore(patch): release 1.23.0-beta.7 on beta channel [skip ci]
  fix: correct some tooling and dependency issues (#749)
  chore(minor): release 1.23.0-beta.6 on beta channel [skip ci]
  fix: add missing title props
  feat: add Edit Page button to insight pages (#738)
  feat: enable repository selection & add to new insights page (#693)
  chore(minor): release 1.23.0-beta.5 on beta channel [skip ci]
  ...
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.23.0-beta.6](open-sauced/app@v1.23.0-beta.5...v1.23.0-beta.6) (2023-01-03)

### πŸ• Features

* add Edit Page button to insight pages ([#738](open-sauced/app#738)) ([bde1eb8](open-sauced/app@bde1eb8)), closes [#685](open-sauced/app#685)
* enable repository selection & add to new insights page ([#693](open-sauced/app#693)) ([ba6405d](open-sauced/app@ba6405d)), closes [#593](open-sauced/app#593) [#592](open-sauced/app#592)

### πŸ› Bug Fixes

* add missing title props ([cdcc28d](open-sauced/app@cdcc28d))
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.23.0](open-sauced/app@v1.22.2...v1.23.0) (2023-01-05)

### πŸ§‘β€πŸ’» Code Refactoring

* update pr velocity indicator ([#707](open-sauced/app#707)) ([7e96735](open-sauced/app@7e96735)), closes [#684](open-sauced/app#684)

### πŸ› Bug Fixes

* add missing title props ([cdcc28d](open-sauced/app@cdcc28d))
* correct some tooling and dependency issues ([#749](open-sauced/app#749)) ([cb4ec9f](open-sauced/app@cb4ec9f))
* only pull insights for stacked avatar on insight pages ([#761](open-sauced/app#761)) ([80ae119](open-sauced/app@80ae119))
* overflow x bug on dashboard ([#709](open-sauced/app#709)) ([b022dbc](open-sauced/app@b022dbc)), closes [#677](open-sauced/app#677)
* user profile auth avatar processing errors ([#755](open-sauced/app#755)) ([3ba87a6](open-sauced/app@3ba87a6)), closes [#733](open-sauced/app#733)

### πŸ• Features

* add Edit Page button to insight pages ([#738](open-sauced/app#738)) ([bde1eb8](open-sauced/app@bde1eb8)), closes [#685](open-sauced/app#685)
* add favorite repos on the design system ([#744](open-sauced/app#744)) ([c8660b5](open-sauced/app@c8660b5)), closes [#717](open-sauced/app#717)
* add hover card and expand on contributor stack ([#742](open-sauced/app#742)) ([9f45b3d](open-sauced/app@9f45b3d))
* adjust search font-size ([c451450](open-sauced/app@c451450))
* change name to enhace from logarithmic ([#743](open-sauced/app#743)) ([4384324](open-sauced/app@4384324))
* enable repository selection & add to new insights page ([#693](open-sauced/app#693)) ([ba6405d](open-sauced/app@ba6405d)), closes [#593](open-sauced/app#593) [#592](open-sauced/app#592)
* Filter dashboard scatter plot by PR states ([#736](open-sauced/app#736)) ([f04093a](open-sauced/app@f04093a))
* handle close modal when click outside and improve select usage ([#705](open-sauced/app#705)) ([105a47b](open-sauced/app@105a47b)), closes [#689](open-sauced/app#689)
* update icon PR  details row ([#706](open-sauced/app#706)) ([dac42ba](open-sauced/app@dac42ba)), closes [#696](open-sauced/app#696)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Save to dashboard feature bug: Click to un-filter by repo
4 participants