-
Notifications
You must be signed in to change notification settings - Fork 90
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
Catalog: GraphQL for packages #2552
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2552 +/- ##
==========================================
- Coverage 42.93% 42.93% -0.01%
==========================================
Files 520 532 +12
Lines 24677 24621 -56
Branches 3315 3332 +17
==========================================
- Hits 10595 10570 -25
- Misses 13258 13286 +28
+ Partials 824 765 -59
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@fiskus this one is in working state and deployed to searchminimal along with the related backend changes |
const { sort, filter, p } = parseSearch(location.search) | ||
const page = p && parseInt(p, 10) | ||
const { sort, filter, p } = parseSearch(location.search, true) | ||
const page = p ? parseInt(p, 10) : undefined |
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 see multiple undefined
in this module. Can we use null
here? I think that it's more straightforward to use null
, and leave undefined
only when it's really undefined
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.
Can we use null here?
here -- yes, but not everywhere (e.g. not as a title
attribute for a span
, since it only accepts string | undefined
)
I think that it's more straightforward to use null, and leave undefined only when it's really undefined
tbh i don't feel much difference in most cases 🤷 (with a notable exception of JSON.strignify
)
catalog/app/containers/Bucket/PackageRevisions/PackageRevisions.tsx
Outdated
Show resolved
Hide resolved
29e18cb
to
8b52099
Compare
@fiskus i've rebased to incorporate your PackageTree changes, pls see if everything's ok |
@@ -36,6 +36,7 @@ export function viewModeToSelectOption(m: ViewMode | null): SelectOption | null | |||
export function useViewModes( | |||
path: string, | |||
modeInput: string | null | undefined, | |||
// XXX: consider using a plain boolean here since the contents of this object are unused |
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.
It's a deliberate choice. Reusing a small number of data structures reduces cognitive load. Also, it reduces footprint because it doesn't copy data, just passes the link to the object.
cdd3b7b
to
4c170e9
Compare
Description
containers/Bucket
utils/useQuery
helpercontainers/Bucket/requests/requestsUntyped.js
TODO