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

Catalog: Admin: Tabulator Settings (open query) + docs #4255

Merged
merged 28 commits into from
Dec 18, 2024

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Dec 6, 2024

Description

admin-tabulator-settings

TODO

  • Documentation
    • run optipng on any new PNGs
    • JavaScript: basic explanation and screenshot of new features
    • Markdown somewhere in docs/**/*.md that explains the feature to end users (said .md files should be linked from SUMMARY.md so they appear on https://docs.quiltdata.com)
  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

Sorry, something went wrong.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 40 lines in your changes missing coverage. Please review.

Project coverage is 38.27%. Comparing base (4a0a562) to head (cda8c8d).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...pp/containers/Admin/Settings/TabulatorSettings.tsx 0.00% 31 Missing ⚠️
catalog/app/utils/GraphQL/Provider.tsx 0.00% 4 Missing ⚠️
...in/Settings/gql/SetTabulatorOpenQuery.generated.ts 0.00% 2 Missing ⚠️
...Admin/Settings/gql/TabulatorOpenQuery.generated.ts 0.00% 2 Missing ⚠️
catalog/app/containers/Admin/Settings/Settings.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4255      +/-   ##
==========================================
- Coverage   38.31%   38.27%   -0.05%     
==========================================
  Files         773      776       +3     
  Lines       34283    34323      +40     
  Branches     5215     5219       +4     
==========================================
  Hits        13137    13137              
- Misses      20603    20643      +40     
  Partials      543      543              
Flag Coverage Δ
api-python 91.29% <ø> (ø)
catalog 16.87% <0.00%> (-0.03%) ⬇️
lambda 91.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nl0 added 3 commits December 10, 2024 13:37
cl
@nl0 nl0 changed the title Tabulator feature flag Catalog: Admin: Tabulator Settings (unrestricted access) Dec 11, 2024
@nl0
Copy link
Member Author

nl0 commented Dec 11, 2024

@drernie i'd appreciate some help with the documentation

@nl0 nl0 requested review from Copilot and fiskus December 11, 2024 14:59

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 14 changed files in this pull request and generated no suggestions.

Files not reviewed (8)
  • catalog/app/containers/Admin/Settings/gql/SetTabulatorUnrestricted.graphql: Language not supported
  • catalog/app/containers/Admin/Settings/gql/TabulatorUnrestricted.graphql: Language not supported
  • shared/graphql/schema.graphql: Language not supported
  • .github/workflows/deploy-catalog.yaml: Evaluated as low risk
  • catalog/CHANGELOG.md: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/Settings.tsx: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/TabulatorSettings.tsx: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/gql/SetTabulatorUnrestricted.generated.ts: Evaluated as low risk
fiskus
fiskus previously approved these changes Dec 11, 2024
Copy link
Member

@fiskus fiskus left a comment

Choose a reason for hiding this comment

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

I have to admit, seeing the * in the CSS scared me a little at first. But looking closer, it’s actually clever and even performant!

On the other hand, I’m still struggling to see the benefits of using Effect here. It seems to make the code more verbose and less idiomatic compared to simpler approaches like comparing to null. I feel I’ve expressed my concerns about Effect clearly at this point, so I won’t bring it up again.

That said, the implementation is solid, and I appreciate the attention to detail.

nl0 added 2 commits December 11, 2024 17:33
@nl0 nl0 requested a review from drernie December 11, 2024 16:42
@nl0
Copy link
Member Author

nl0 commented Dec 12, 2024

I have to admit, seeing the * in the CSS scared me a little at first. But looking closer, it’s actually clever and even performant!

i was sure it's an established pattern =)

On the other hand, I’m still struggling to see the benefits of using Effect here. It seems to make the code more verbose and less idiomatic compared to simpler approaches like comparing to null. I feel I’ve expressed my concerns about Effect clearly at this point, so I won’t bring it up again.

fair enough

That said, the implementation is solid, and I appreciate the attention to detail.

thanks

@nl0 nl0 marked this pull request as ready for review December 12, 2024 09:29
@nl0 nl0 requested review from fiskus, sir-sigurd and Copilot December 12, 2024 09:29
@nl0
Copy link
Member Author

nl0 commented Dec 12, 2024

@sir-sigurd @fiskus pls see if the documentation changes make sense

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 14 changed files in this pull request and generated no suggestions.

Files not reviewed (8)
  • catalog/app/containers/Admin/Settings/gql/SetTabulatorUnrestricted.graphql: Language not supported
  • catalog/app/containers/Admin/Settings/gql/TabulatorUnrestricted.graphql: Language not supported
  • shared/graphql/schema.graphql: Language not supported
  • catalog/app/containers/Admin/Settings/Settings.tsx: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/TabulatorSettings.tsx: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/gql/SetTabulatorUnrestricted.generated.ts: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/gql/TabulatorUnrestricted.generated.ts: Evaluated as low risk
  • catalog/app/model/graphql/schema.generated.ts: Evaluated as low risk
nl0 added 2 commits December 12, 2024 10:32
This reverts commit bf2fca5.
nl0 added 2 commits December 13, 2024 17:38
@nl0
Copy link
Member Author

nl0 commented Dec 13, 2024

@QuiltSimon @drernie @sir-sigurd @akarve pls see if the documentation is good enough

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
nl0 added 10 commits December 17, 2024 15:29
* master:
  `quilt_summarize.json` editor (#4254)
This reverts commit 09fefa9.
@nl0
Copy link
Member Author

nl0 commented Dec 18, 2024

i think this is now ready

@nl0 nl0 changed the title Catalog: Admin: Tabulator Settings (unrestricted access) Catalog: Admin: Tabulator Settings (open query) + docs Dec 18, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 14 changed files in this pull request and generated no comments.

Files not reviewed (7)
  • catalog/app/containers/Admin/Settings/gql/SetTabulatorOpenQuery.graphql: Language not supported
  • catalog/app/containers/Admin/Settings/gql/TabulatorOpenQuery.graphql: Language not supported
  • shared/graphql/schema.graphql: Language not supported
  • catalog/CHANGELOG.md: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/Settings.tsx: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/TabulatorSettings.tsx: Evaluated as low risk
  • catalog/app/containers/Admin/Settings/gql/SetTabulatorOpenQuery.generated.ts: Evaluated as low risk
@nl0 nl0 mentioned this pull request Dec 18, 2024
@nl0 nl0 enabled auto-merge December 18, 2024 14:34
@nl0 nl0 added this pull request to the merge queue Dec 18, 2024
Merged via the queue into master with commit e7743a7 Dec 18, 2024
36 of 38 checks passed
@nl0 nl0 deleted the tabulator-feature-flag branch December 18, 2024 14:50
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

4 participants