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

ref: Refactor context switcher #2292

Merged
merged 6 commits into from
Oct 6, 2023
Merged

ref: Refactor context switcher #2292

merged 6 commits into from
Oct 6, 2023

Conversation

RulaKhaled
Copy link
Contributor

Description

This PR is meant to handle the following in context switcher:

  • We are prob drilling owner to use for current active context
  • Something that we noticed recently for people that have over 20 orgs (see here), the page breaks when they default to an org that doesn't exist in the first query page, as we're fetching the first 20 org and checking if the org exists for the user.

Solution:

  • Directly use useParams to fetch owner from route, this is the current active context.
  • Use use owner to fetch owner details, if user has no access owner page should default to not found, we can't simply use my orgs because it's paginated and it is not guaranteed to exist on time of checking for current active context .

Notable Changes

  • Remove the use of activeContext at all
  • Remove the use of getCurrentContext
  • Use useParams and useOwner to handle current user

Screenshots

nothing visual changed, please check the preview deploy to test owner page and it's children.

Link to Sample Entry

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@codecov-staging
Copy link

codecov-staging bot commented Sep 28, 2023

Codecov Report

Merging #2292 (844a14f) into main (8055e0e) will decrease coverage by 0.21%.
Report is 6 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 844a14f differs from pull request most recent head 2f364a2. Consider uploading reports for the commit 2f364a2 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
- Coverage   98.02%   97.82%   -0.21%     
==========================================
  Files         704      705       +1     
  Lines        8207     8216       +9     
  Branches     1980     1983       +3     
==========================================
- Hits         8045     8037       -8     
- Misses        160      177      +17     
  Partials        2        2              
Files Coverage Δ
...rc/layouts/MyContextSwitcher/MyContextSwitcher.jsx 100.00% <100.00%> (ø)
src/pages/AccountSettings/shared/Header/Header.jsx 100.00% <ø> (ø)
src/pages/AnalyticsPage/AnalyticsPage.jsx 100.00% <ø> (ø)
src/pages/AnalyticsPage/Header/Header.jsx 100.00% <100.00%> (ø)
src/pages/MembersPage/Header/Header.jsx 100.00% <100.00%> (ø)
src/pages/OwnerPage/Header/Header.jsx 100.00% <ø> (ø)
src/pages/PlanPage/Header/Header.jsx 100.00% <100.00%> (ø)
src/ui/Avatar/Avatar.jsx 100.00% <100.00%> (ø)
src/ui/ContextSwitcher/ContextSwitcher.jsx 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8055e0e...2f364a2. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #2292 (93c6176) into main (ba095ab) will increase coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2292     +/-   ##
=======================================
+ Coverage   34.36   34.40   +0.04     
=======================================
  Files        656     656             
  Lines       8096    8087      -9     
  Branches    1992    1985      -7     
=======================================
  Hits        2782    2782             
+ Misses      5285    5282      -3     
+ Partials      29      23      -6     
Files Coverage Δ
src/pages/AccountSettings/shared/Header/Header.jsx 0.00% <ø> (ø)
src/pages/AnalyticsPage/AnalyticsPage.jsx 0.00% <ø> (ø)
src/pages/OwnerPage/Header/Header.jsx 0.00% <ø> (ø)
src/pages/MembersPage/Header/Header.jsx 0.00% <0.00%> (ø)
src/pages/PlanPage/Header/Header.jsx 0.00% <0.00%> (ø)
src/ui/Avatar/Avatar.jsx 0.00% <0.00%> (ø)
src/pages/AnalyticsPage/Header/Header.jsx 0.00% <0.00%> (ø)
...rc/layouts/MyContextSwitcher/MyContextSwitcher.jsx 0.00% <0.00%> (ø)
src/ui/ContextSwitcher/ContextSwitcher.jsx 0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba095ab...93c6176. Read the comment docs.

@codecov-public-qa
Copy link

codecov-public-qa bot commented Sep 28, 2023

Codecov Report

Merging #2292 (93c6176) into main (ba095ab) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
+ Coverage   34.36%   34.40%   +0.03%     
==========================================
  Files         656      656              
  Lines        8096     8087       -9     
  Branches     1942     1942              
==========================================
  Hits         2782     2782              
+ Misses       5291     5282       -9     
  Partials       23       23              
Files Coverage Δ
src/pages/AccountSettings/shared/Header/Header.jsx 0.00% <ø> (ø)
src/pages/AnalyticsPage/AnalyticsPage.jsx 0.00% <ø> (ø)
src/pages/OwnerPage/Header/Header.jsx 0.00% <ø> (ø)
src/pages/MembersPage/Header/Header.jsx 0.00% <0.00%> (ø)
src/pages/PlanPage/Header/Header.jsx 0.00% <0.00%> (ø)
src/ui/Avatar/Avatar.jsx 0.00% <0.00%> (ø)
src/pages/AnalyticsPage/Header/Header.jsx 0.00% <0.00%> (ø)
...rc/layouts/MyContextSwitcher/MyContextSwitcher.jsx 0.00% <0.00%> (ø)
src/ui/ContextSwitcher/ContextSwitcher.jsx 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba095ab...93c6176. Read the comment docs.

@netlify
Copy link

netlify bot commented Sep 28, 2023

Deploy Preview for gazebo-staging ready!

Name Link
🔨 Latest commit 93c6176
🔍 Latest deploy log https://app.netlify.com/sites/gazebo-staging/deploys/651edd8011366200081ad21c
😎 Deploy Preview https://deploy-preview-2292--gazebo-staging.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 configuration.

@RulaKhaled RulaKhaled marked this pull request as ready for review September 28, 2023 12:58
@codecov-qa
Copy link

codecov-qa bot commented Oct 3, 2023

Codecov Report

Merging #2292 (93c6176) into main (ba095ab) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
+ Coverage   34.36%   34.40%   +0.03%     
==========================================
  Files         656      656              
  Lines        8096     8087       -9     
  Branches     1985     1985              
==========================================
  Hits         2782     2782              
+ Misses       5285     5282       -3     
+ Partials       29       23       -6     
Files Coverage Δ
src/pages/AccountSettings/shared/Header/Header.jsx 0.00% <ø> (ø)
src/pages/AnalyticsPage/AnalyticsPage.jsx 0.00% <ø> (ø)
src/pages/OwnerPage/Header/Header.jsx 0.00% <ø> (ø)
src/pages/MembersPage/Header/Header.jsx 0.00% <0.00%> (ø)
src/pages/PlanPage/Header/Header.jsx 0.00% <0.00%> (ø)
src/ui/Avatar/Avatar.jsx 0.00% <0.00%> (ø)
src/pages/AnalyticsPage/Header/Header.jsx 0.00% <0.00%> (ø)
...rc/layouts/MyContextSwitcher/MyContextSwitcher.jsx 0.00% <0.00%> (ø)
src/ui/ContextSwitcher/ContextSwitcher.jsx 0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba095ab...93c6176. Read the comment docs.

orgUsername?.toLowerCase() ===
currentContext?.owner?.username?.toLowerCase()
function ContextItem({ context, defaultOrgUsername, setToggle }) {
const { owner } = useParams()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also pull out the useParams here, it's in the same vein of using services you're tying this component to the URL when it should be agnostic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it! i was mainly focused on getting the use of active context right, but it looks like this component could use some more love

Copy link
Contributor

@nicholas-codecov nicholas-codecov left a comment

Choose a reason for hiding this comment

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

Looks all good to me 👍

@RulaKhaled RulaKhaled merged commit 1e765a3 into main Oct 6, 2023
28 of 31 checks passed
@RulaKhaled RulaKhaled deleted the refactor-org-selector branch October 6, 2023 11:07
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.

None yet

2 participants