-
Notifications
You must be signed in to change notification settings - Fork 583
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
[1.3.1 Bug fixes] Field visibility improvements #3154
Conversation
…hema documentation selection mode is default now for filed visibility
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #3154 +/- ##
============================================
- Coverage 49.25% 15.12% -34.14%
============================================
Files 230 560 +330
Lines 34424 68959 +34535
Branches 325 597 +272
============================================
- Hits 16956 10428 -6528
- Misses 17468 58531 +41063
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Looks good! Left some high level comments and suggestions. The one case statement that does not return is the only critical thing from what I saw. I recommend adding eslint to your editor, and looking at some of the dependency warnings in useSchemaSettings
. It looks like some dependencies may be missing.
It would be interesting see how the useSchemaSettings
hook could be broken out, and perhaps (in part) moved into recoil data flow. But not important now
app/packages/core/src/components/Schema/SchemaSelectControls.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/components/Schema/SchemaSelectControls.tsx
Outdated
Show resolved
Hide resolved
Looking good to me. Only one minor thing with this the json formatter. It doesn't seem to still format. We can remove the format button if it's not a required feature. |
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.
Tested. LGTM
@@ -8,6 +8,14 @@ export default defineConfig({ | |||
coverage: { | |||
reporter: ["json", "lcov"], | |||
reportsDirectory: "./coverage", | |||
enabled: true, | |||
all: true, | |||
exclude: [ |
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.
Nice
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.
LGTM 🚀
* Refactor schemaControls, use selectField() when searching, cleaner schema documentation selection mode is default now for filed visibility * update count dependency to refelct when applying filter rules * add documentation links * better disable field handling - (un)select is broken - fixing next * improve select all control * uncheck subpaths even if they are disabled * show embedded document in selection mode * field doc type using tertiary text color * minor tweaks in color modal title style * review comments and fix include nested fields selection view * review comments, refactor selection row, and count fix * add first vitest for visibility, upgrade vitest packages * add basic test and refactor more * keep one embed doc field * no coverage when yarn test --------- Co-authored-by: manivoxel51 <mani@voxel51@gmail.com> Co-authored-by: Lanny W <lanzhenwang9@gmail.com>
What changes are proposed in this pull request?
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyone
Python library changes