Nc fix/UI changes #10079
Nc fix/UI changes #10079
Conversation
|
Warning Rate limit exceeded@o1lab has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (16)
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a series of UI and styling enhancements across multiple components in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue (2)
Line range hint
1-391: Consider architectural improvements for maintainabilityWhile the current implementation is functional, consider the following improvements for future iterations:
- Extract the group management logic into a composable to separate concerns
- Move hardcoded error messages to translation files
- Clean up commented-out code (around line 300) or document why it's preserved
Would you like me to provide example code for any of these suggestions?
Line range hint
136-136: Consider adding explicit type for drag eventBased on the team's preference for explicit type annotations, consider creating a dedicated type for the drag event:
interface DragEvent { moved: { newIndex: number oldIndex: number } }Then update the method signature:
const onMove = async (event: DragEvent) => {packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue (1)
2659-2665: Add accessibility attributes to the icon-only buttonWhile the button styling is consistent with the design system, consider adding accessibility attributes for better screen reader support.
Consider adding these accessibility improvements:
<NcButton v-if="!isMobileMode" size="small" class="!rounded-l-none nc-add-record-more-info" type="secondary" :shadow="false" + aria-label="More options for adding new record" + :title="$t('tooltip.moreOptions')" > <GeneralIcon icon="arrowUp" /> </NcButton>packages/nc-gui/components/smartsheet/grid/Table.vue (2)
2612-2614: Ensure consistent spacing in pagination controlsThe new div container for pagination controls uses absolute positioning which may cause layout issues on smaller screens. Consider using a more flexible positioning approach.
- <div v-if="headerOnly !== true && paginationDataRef && !isGroupBy" class="absolute bottom-12 left-2" @click.stop> + <div v-if="headerOnly !== true && paginationDataRef && !isGroupBy" class="fixed bottom-12 left-2 z-10" @click.stop>
2622-2623: Consistent button shadow removalThe shadow property is being explicitly set to false for multiple buttons. This is good for consistency but could be handled more efficiently through a common button class or mixin.
Consider creating a shared class:
+ <style lang="scss"> + .no-shadow-button { + :deep(.ant-btn) { + box-shadow: none !important; + } + } + </style>Then apply it to buttons:
- :shadow="false" + class="no-shadow-button"Also applies to: 2637-2638
packages/nc-gui/components/feed/Header.vue (1)
4-4: Reduced padding for a more compact header.
Confirm that this does not negatively impact visual clarity or clickable areas.packages/nc-gui/assets/style.scss (1)
1083-1094: LGTM! Consider adding vendor prefixes for better browser compatibility.The animation implementation is clean and enhances the user experience with smooth padding transitions.
Consider adding vendor prefixes for broader browser support:
@keyframes sidebar-node-input-padding-grow { from { padding-left: 0; } to { padding-left: 6px; } } +@-webkit-keyframes sidebar-node-input-padding-grow { + from { + padding-left: 0; + } + to { + padding-left: 6px; + } +} .animate-sidebar-node-input-padding { animation: sidebar-node-input-padding-grow 0.2s ease forwards; + -webkit-animation: sidebar-node-input-padding-grow 0.2s ease forwards; }packages/nc-gui/utils/iconUtils.ts (1)
2928-2930: Consider enhancing searchable keywords for better discoverabilityWhile the basic keywords are present, consider adding more descriptive keywords for the new heading icons to improve searchability. For example:
- For headings: "title", "section", "header", "typography", "format"
- For quotes: "blockquote", "citation", "reference", "text", "format"
ncHeading1: { icon: NcHeading1, - keywords: ['heading', 'h1'], + keywords: ['heading', 'h1', 'title', 'section', 'header', 'typography', 'format'], }, ncHeading2: { icon: NcHeading2, - keywords: ['heading', 'h2'], + keywords: ['heading', 'h2', 'subtitle', 'section', 'header', 'typography', 'format'], }, ncHeading3: { icon: NcHeading3, - keywords: ['heading', 'h3'], + keywords: ['heading', 'h3', 'subsection', 'section', 'header', 'typography', 'format'], }, ncQuote: { icon: NcQuote, - keywords: ['quotes'], + keywords: ['quotes', 'blockquote', 'citation', 'reference', 'text', 'format'], },Also applies to: 3752-3768
packages/nc-gui/components/feed/Changelog/Item.vue (1)
136-138: Consider truncating date format on smaller screensWhile preventing date wrapping is good, consider using a more compact date format on mobile screens (e.g., "DD/MM/YY") to prevent potential overflow issues.
- <span class="font-medium text-sm text-gray-500 whitespace-nowrap"> - {{ dayjs(CreatedAt).format('MMM DD, YYYY') }} + <span class="font-medium text-sm text-gray-500 whitespace-nowrap"> + {{ dayjs(CreatedAt).format(isSmallScreen ? 'DD/MM/YY' : 'MMM DD, YYYY') }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/nc-gui/assets/nc-icons-v2/check-list.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/heading-1.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/heading-2.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/heading-3.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/number-list.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/quote.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons/megaphone.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (15)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/cell/RichText/SelectedBubbleMenu.vue(13 hunks)packages/nc-gui/components/dashboard/TreeView/ProjectNode.vue(5 hunks)packages/nc-gui/components/dashboard/TreeView/ViewsNode.vue(1 hunks)packages/nc-gui/components/feed/Changelog/Item.vue(3 hunks)packages/nc-gui/components/feed/Header.vue(1 hunks)packages/nc-gui/components/feed/Social.vue(1 hunks)packages/nc-gui/components/smartsheet/details/Fields.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(3 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(3 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(3 hunks)packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue(2 hunks)packages/nc-gui/components/workspace/View.vue(3 hunks)packages/nc-gui/utils/iconUtils.ts(6 hunks)packages/nc-gui/windi.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/nc-gui/components/workspace/View.vue
🧰 Additional context used
📓 Learnings (1)
packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue (1)
Learnt from: DarkPhoenix2704
PR: nocodb/nocodb#8781
File: packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue:136-136
Timestamp: 2024-11-12T03:23:04.401Z
Learning: DarkPhoenix2704 prefers explicit type annotations in TypeScript for clarity, even when the type could be inferred.
🔇 Additional comments (23)
packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue (2)
252-258: LGTM! UI consistency improvement
The explicit shadow property setting aligns with the UI refinement objectives of this PR, ensuring consistent button styling across the draggable interface.
322-322: LGTM! Consistent styling maintained
The shadow property setting on the remove button maintains visual consistency with other buttons in the component.
packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue (2)
2634-2635: LGTM: Button shadow removal is consistent with design patterns
The removal of shadow from the button helps maintain visual consistency across the UI.
2648-2649: LGTM: Consistent button styling applied
The shadow removal is consistently applied across the button group, maintaining visual harmony.
packages/nc-gui/components/smartsheet/grid/Table.vue (1)
2648-2654: Review mobile responsiveness of the new button
The new button component has specific styling for non-mobile mode but might need additional adjustments for mobile view.
Let's verify the mobile layout:
✅ Verification successful
Mobile responsiveness is properly handled
The button is correctly implemented with proper mobile responsiveness:
- The button is conditionally rendered with
v-if="!isMobileMode", meaning it's hidden on mobile - Mobile view has its own dedicated button with
v-if="isMobileMode"(line 2648) - The table component has comprehensive mobile-specific styles:
- Mobile-specific row heights (56px)
- Hidden elements on mobile using
!xs:hiddenutility classes - Mobile-specific layout classes and conditional rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for mobile-specific styles and responsive design patterns
# Look for mobile-specific classes and media queries
rg -A 3 "isMobileMode|xs:|sm:|md:" packages/nc-gui/components/smartsheet/grid/
Length of output: 17192
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (3)
675-675: Good consistency in disabling button shadow.
No issues with removing the button shadow property here.
688-688: Consistent change for button styling.
Similarly, removing shadow here matches the styling on line 675.
701-701: Uniform button styling across all calendar styling toggles.
This maintains consistency and simplifies the UI theme.
packages/nc-gui/components/feed/Header.vue (3)
5-5: Gap size reduced for denser layout.
Ensure that the smaller gap still provides sufficient spacing for readability.
6-6: Enlarged icon dimensions.
Increasing the icon size can improve visibility, but verify it does not overlap neighboring UI elements on narrower viewports.
8-8: Adjusted text styling to bold and smaller font size.
This change rebalances the header’s hierarchy. Confirm if it remains prominent enough, given the smaller text size.
packages/nc-gui/components/feed/Social.vue (1)
48-48: Added padding to the social icons container.
This helps create a cleaner layout. Consider verifying it does not introduce any unintended scrollbars in smaller containers.
packages/nc-gui/windi.config.ts (1)
125-125: New custom box shadow “nc-sm” added.
It looks like a subtle shadow. Double-check that usage in conjunction with existing shadows remains visually consistent across the UI.
packages/nc-gui/components/dashboard/TreeView/ViewsNode.vue (1)
355-355: LGTM! Enhanced input field styling.
The changes improve the input field's visual feedback and user experience:
- Added rounded corners with
!rounded-md - Consistent height with
!h-6 - Proper padding with
!pr-1.5 - Smooth animation with
animate-sidebar-node-input-padding
packages/nc-gui/components/cell/RichText/SelectedBubbleMenu.vue (2)
Line range hint 167-374: LGTM! Standardized icon usage.
Replaced various icon libraries with the unified GeneralIcon component, improving consistency and maintainability:
- Replaced format icons (bold, italic, underline, strike)
- Replaced list icons (bullet, numbered, task)
- Replaced heading icons (h1, h2, h3)
- Replaced utility icons (code, quote)
432-434: LGTM! Simplified close button structure.
The close button template structure has been simplified while maintaining the same functionality:
- Cleaner conditional rendering
- Consistent styling with
!stickyandpr-0.5
packages/nc-gui/components/dashboard/TreeView/ProjectNode.vue (2)
574-578: LGTM! Input field implementation follows Vue best practices.
The input field implementation is well-structured with proper two-way binding and consistent styling.
800-807: LGTM! Expand icon implementation handles state changes correctly.
The expand icon template properly handles the source rename state and maintains UI consistency.
packages/nc-gui/components/smartsheet/details/Fields.vue (1)
1257-1257: LGTM! Button styling follows design system guidelines.
The button wrapper styling maintains consistency with the design system using proper border-radius classes.
packages/nc-gui/utils/iconUtils.ts (2)
309-310: LGTM: New icon imports are properly organized
The new icon imports are correctly grouped with related icons and follow the existing import pattern.
Also applies to: 590-594
1134-1135: LGTM: Icon mappings are consistent with existing patterns
The new icon mappings in iconMap follow the established naming convention and are properly organized with related icons.
Also applies to: 1395-1398
packages/nc-gui/components/feed/Changelog/Item.vue (2)
101-102: LGTM! Improved badge container layout
The addition of flex-wrap and consistent spacing improves the responsive behavior and visual consistency of the badge container.
113-113: Verify tag layout on mobile devices
While the spacing and nowrap improvements enhance desktop view, please verify that:
- Long tags don't overflow on mobile screens
- The gap-2 spacing works well with different tag lengths
Also applies to: 130-130
✅ Verification successful
Let me generate additional scripts to check for responsive styles and mobile-specific handling in the codebase.
Tag layout is properly handled for mobile devices
The codebase shows proper handling of text overflow and responsive layout:
- The tag text has
whitespace-nowrapclass to prevent wrapping (line 130) - The parent container has
flex gap-2which provides consistent spacing (line 113) - The component includes proper overflow handling with
overflow-y-autoand custom scrollbar - Text truncation is managed programmatically via the
truncateref
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for responsive design tests or mobile-specific styles
rg -g '*.{vue,ts,js,scss}' -A 3 'mobile|sm:|xs:|@media.*max-width'
Length of output: 89590
Script:
#!/bin/bash
# Search for responsive styles and mobile-specific handling in the component and related files
rg -g '*.{vue,scss,css}' -A 3 'sm:|xs:|@media.*max-width|mobile' packages/nc-gui/components/feed/Changelog/
# Search for any overflow handling styles
rg -g '*.{vue,scss,css}' -A 3 'overflow|truncate|text-overflow|whitespace-nowrap' packages/nc-gui/components/feed/Changelog/
Length of output: 3583
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/nocodb/nocodb/pull/10079 ⚙️ Updating now by workflow run 12428058077. What is Uffizzi? Learn more! |
o1lab
force-pushed
the
nc-fix/ui-changes-from-shreyas
branch
from
8b07177 to
a9bf39b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (3)
Line range hint
89-117: Add loading state management for cover image updatesThe
updateCoverImagemethod performs multiple async operations without managing loading states, which could lead to race conditions if users make rapid changes.const updateCoverImage = async (val?: string | null) => { + const loadingState = ref(false) try { + loadingState.value = true if ( (activeView.value?.type === ViewTypes.GALLERY || activeView.value?.type === ViewTypes.KANBAN || activeView.value?.type === ViewTypes.CALENDAR) && activeView.value?.id && activeView.value?.view ) { // ... existing code ... } } catch (error) { console.error('Failed to update cover image:', error) + message.error(t('error.failedToUpdateCoverImage')) + } finally { + loadingState.value = false } }
Line range hint
308-320: Enhance accessibility for field visibility toggleThe field visibility toggle lacks proper ARIA labels and keyboard navigation support.
<div v-e="['a:fields:show-hide']" class="flex flex-row items-center w-full truncate ml-1 py-[5px] pr-2" + role="button" + :tabindex="isLocked ? -1 : 0" + :aria-label="$t('aria.toggleFieldVisibility', { field: field.title })" :class="{ 'cursor-pointer': !isLocked, }" - @click="() => { + @click.prevent="() => { if (isLocked) return field.show = !field.show toggleFieldVisibility(field.show, field) }" + @keydown.enter="() => { + if (isLocked) return + field.show = !field.show + toggleFieldVisibility(field.show, field) + }" >
Event bus cleanup is missing and needs to be added
The codebase shows a consistent pattern of using
onBeforeUnmountfor cleanup in various components, including event listeners and hooks. Looking at similar components likeGroupBy.vuewhich properly cleans up its hooks usingonBeforeUnmount, we can confirm that the review comment's concern about potential memory leaks is valid.
- Add cleanup code in
FieldsMenu.vueto unsubscribe from event bus events before component unmount- Follow the established pattern seen in other components for proper event cleanup
🔗 Analysis chain
Line range hint
47-55: Verify event bus cleanup on component unmountThe event bus listeners are set up but there's no cleanup when the component is unmounted, which could lead to memory leaks.
+import { onBeforeUnmount } from 'vue' + eventBus.on((event) => { if (event === SmartsheetStoreEvents.FIELD_RELOAD) { loadViewColumns() } else if (event === SmartsheetStoreEvents.MAPPED_BY_COLUMN_CHANGE) { loadViewColumns() } }) + +onBeforeUnmount(() => { + eventBus.off(SmartsheetStoreEvents.FIELD_RELOAD) + eventBus.off(SmartsheetStoreEvents.MAPPED_BY_COLUMN_CHANGE) +})🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components that might have similar event bus patterns ast-grep --pattern 'eventBus.on($$$)' # Check if there are any cleanup patterns in the codebase rg -A 5 'onBeforeUnmount|onUnmounted.*eventBus'Length of output: 10333
🧹 Nitpick comments (3)
packages/nc-gui/components/dashboard/TreeView/ProjectNode.vue (1)
800-807: Consider simplifying the condition for better readability.The current nested condition could be simplified to improve maintainability.
-v-if=" - !( - header?.[0]?.props?.['data-sourceId'] && - sourceRenameHelpers[header?.[0]?.props?.['data-sourceId']]?.editMode - ) -" +v-if="!header?.[0]?.props?.['data-sourceId'] || !sourceRenameHelpers[header?.[0]?.props?.['data-sourceId']]?.editMode"packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue (2)
675-675: Ensure consistent shadow property usage across styling buttonsThe shadow property is duplicated across three similar button components. Consider extracting common button properties into a shared configuration object or component.
- :shadow="false" - :disabled="isLocked" - @click.stop="toggleFieldStyles(field, 'bold', !field.bold)" + v-bind="textStyleButtonProps" + @click.stop="toggleFieldStyles(field, 'bold', !field.bold)" // Add to script section: const textStyleButtonProps = { shadow: false, disabled: isLocked, }Also applies to: 688-688, 701-701
Line range hint
416-424: Optimize field filtering performanceThe filtering logic in the template is being recomputed on every render. Consider memoizing the filtered fields using a computed property.
+ const visibleFields = computed(() => { + return filteredFieldList.value + .filter((el) => (activeView.value.type !== ViewTypes.CALENDAR ? el !== gridDisplayValueField.value : true)) + }) - v-if="filteredFieldList.filter((el) => (activeView.type !== ViewTypes.CALENDAR ? el !== gridDisplayValueField : true)).includes(field)" + v-if="visibleFields.includes(field)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
packages/nc-gui/assets/nc-icons-v2/check-list.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/code-block.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/heading-1.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/heading-2.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/heading-3.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/number-list.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons-v2/quote.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/assets/nc-icons/megaphone.svgis excluded by!**/*.svg,!**/*.svgpackages/nc-gui/lang/en.jsonis excluded by!**/*.json
📒 Files selected for processing (15)
packages/nc-gui/assets/style.scss(1 hunks)packages/nc-gui/components/cell/RichText/SelectedBubbleMenu.vue(13 hunks)packages/nc-gui/components/dashboard/TreeView/ProjectNode.vue(5 hunks)packages/nc-gui/components/dashboard/TreeView/ViewsNode.vue(1 hunks)packages/nc-gui/components/feed/Changelog/Item.vue(3 hunks)packages/nc-gui/components/feed/Header.vue(1 hunks)packages/nc-gui/components/feed/Social.vue(1 hunks)packages/nc-gui/components/smartsheet/details/Fields.vue(1 hunks)packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue(3 hunks)packages/nc-gui/components/smartsheet/grid/Table.vue(3 hunks)packages/nc-gui/components/smartsheet/toolbar/FieldsMenu.vue(3 hunks)packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue(2 hunks)packages/nc-gui/components/workspace/View.vue(3 hunks)packages/nc-gui/utils/iconUtils.ts(9 hunks)packages/nc-gui/windi.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/nc-gui/assets/style.scss
- packages/nc-gui/components/feed/Social.vue
- packages/nc-gui/components/feed/Header.vue
- packages/nc-gui/components/cell/RichText/SelectedBubbleMenu.vue
- packages/nc-gui/windi.config.ts
- packages/nc-gui/components/workspace/View.vue
- packages/nc-gui/components/dashboard/TreeView/ViewsNode.vue
- packages/nc-gui/components/smartsheet/grid/InfiniteTable.vue
- packages/nc-gui/components/smartsheet/toolbar/GroupByMenu.vue
- packages/nc-gui/components/feed/Changelog/Item.vue
- packages/nc-gui/components/smartsheet/grid/Table.vue
- packages/nc-gui/components/smartsheet/details/Fields.vue
🔇 Additional comments (8)
packages/nc-gui/components/dashboard/TreeView/ProjectNode.vue (4)
574-578: LGTM! Nice touch with the animation.
The switch to Ant Design's input component is well implemented, and the addition of the animation class provides a smooth visual transition.
823-830: LGTM! Clean implementation of dynamic padding.
The conditional padding and source identification are well implemented.
862-866: LGTM! Consistent implementation with project title input.
The source rename input follows the same pattern as the project title input, with proper event handling and styling.
886-889: LGTM! Clean implementation of options container.
The conditional rendering and flex layout are appropriately implemented.
packages/nc-gui/utils/iconUtils.ts (4)
309-310: LGTM: Clean imports for new list icons
The new icon imports follow the existing pattern and naming conventions.
1135-1136: LGTM: Icon map additions for list types
The new list icons are properly integrated into the iconMap object with consistent naming.
3759-3774: LGTM: Searchable map entries with appropriate keywords
The new icons are properly added to the searchable map with relevant keywords for discovery.
1397-1401: LGTM: New heading and quote icons
The heading (h1-h3) and quote icons are properly integrated into the iconMap object.
Let's verify the icon usage in the codebase:
✅ Verification successful
Icons are properly integrated and used in the rich text editor
The heading (h1-h3) and quote icons are correctly:
- Imported from the
nc-icons-v2package - Mapped in the
iconUtils.tswith proper naming - Used in the rich text editor's bubble menu (
SelectedBubbleMenu.vue) with appropriate keyboard shortcuts and tooltips - Implemented with proper active states and disabled conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where the new heading and quote icons are being used
echo "Checking usage of new heading icons..."
rg -l "ncHeading[123]|ncQuote" --type vue --type ts
echo "Checking for any potential naming conflicts..."
rg -l "heading-[123]|quote" --type vue --type ts
Length of output: 350
Script:
#!/bin/bash
# Let's try searching without file type restrictions since vue might not be recognized
echo "Checking usage of new heading icons..."
rg -l "ncHeading[123]|ncQuote"
echo -e "\nChecking icon imports and definitions..."
rg -l "NcHeading[123]|NcQuote"
echo -e "\nChecking for potential naming conflicts..."
rg -l "heading-[123]|quote"
Length of output: 4467
Script:
#!/bin/bash
# Let's examine the specific usage context in the files where these icons are used
echo "Checking SelectedBubbleMenu.vue implementation..."
cat "packages/nc-gui/components/cell/RichText/SelectedBubbleMenu.vue"
echo -e "\nChecking icon definitions..."
rg -A 2 "NcHeading[123]|NcQuote" "packages/nc-gui/utils/iconUtils.ts"
Length of output: 16094
o1lab
force-pushed
the
nc-fix/ui-changes-from-shreyas
branch
3 times, most recently
from
c7bc9d5 to
5197061
Compare
o1lab
force-pushed
the
nc-fix/ui-changes-from-shreyas
branch
from
5197061 to
ea7d05e
Compare
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of