-
-
Notifications
You must be signed in to change notification settings - Fork 305
fix: update glossary documentation links and temporarily hide import section #3278
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughUpdated documentation links in two EE glossary UI components to point to the managing_glossaries docs; added TODO comments and temporarily commented out the import-terms card and related styling. No public APIs or exported declarations were changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)webapp/src/ee/glossary/components/AddFirstGlossaryMessage.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webapp/src/ee/glossary/components/GlossaryEmptyListMessage.tsx (1)
90-113
: Consider adding a ticket reference to the TODO and verify the commented code approach.The import terms functionality has been temporarily disabled by commenting out the entire UI block. While this aligns with the PR's objective, consider the following:
- TODO specificity: Add a ticket reference or GitHub issue number to track when this feature should be re-enabled (e.g.,
TODO(#1234): enable when docs are ready
).- Unused prop: The
onImport
prop is still accepted by the component but is now unused. This is likely intentional for when the feature is re-enabled, but verify this is the desired approach.- Alternative approach: For longer-term temporary disabling, consider using a feature flag instead of commented code to make the toggle more maintainable.
Since this is a temporary measure as indicated by the TODO, the current approach is acceptable for short-term use. However, if the timeline extends beyond a few weeks, consider refactoring to use a feature flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webapp/src/ee/glossary/components/AddFirstGlossaryMessage.tsx
(1 hunks)webapp/src/ee/glossary/components/GlossaryEmptyListMessage.tsx
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webapp/src/ee/glossary/components/AddFirstGlossaryMessage.tsx (1)
webapp/src/constants/links.tsx (1)
Link
(1-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: BT 🔎 (server-app:runWithoutEeTests)
- GitHub Check: BT 🔎 (security:test)
- GitHub Check: BT 🔎 (ktlint:test)
- GitHub Check: BT 🔎 (server-app:runStandardTests)
- GitHub Check: BT 🔎 (data:test)
- GitHub Check: BT 🔎 (server-app:runWebsocketTests)
- GitHub Check: BT 🔎 (ee-test:test)
- GitHub Check: BT 🔎 (server-app:runContextRecreatingTests)
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (2)
webapp/src/ee/glossary/components/AddFirstGlossaryMessage.tsx (1)
54-54
: LGTM! Documentation link updated correctly.The link now points to the managing_glossaries documentation page, aligning with the PR's objective to update glossary documentation references.
webapp/src/ee/glossary/components/GlossaryEmptyListMessage.tsx (1)
82-82
: LGTM! Documentation link updated correctly.The link now points to the managing_glossaries documentation page, consistent with the changes in AddFirstGlossaryMessage.tsx.
3ca3ff5
to
3d95b53
Compare
3d95b53
to
101d62a
Compare
Summary by CodeRabbit
Documentation
Bug Fixes
Chores