fix:- added elipsis and tooltip to the button content in table widget component #36865
Conversation
WalkthroughThe changes in this pull request focus on enhancing the styling and functionality of 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
Documentation and Community
|
|
hi @rishabhrathod01 can you please review this pr |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (2)
51-72: Excellent work on improving our Button component!You've done a splendid job wrapping our
StyledButtonwith theAutoToolTipComponent. This addition will greatly enhance the user experience by providing a tooltip for button labels that might be too long to display fully.However, let's make one small improvement to our code:
Consider simplifying the conditional rendering. Instead of using a nested ternary operator, we can use the logical AND operator. Here's a suggested change:
- {props.isCellVisible && props.action.isVisible - ? props.action.label && ( - <AutoToolTipComponent - columnType={ColumnTypes.BUTTON} - title={props.action.label} - > - <StyledButton - // ... (button props) - /> - </AutoToolTipComponent> - ) - : null} + {props.isCellVisible && props.action.isVisible && props.action.label && ( + <AutoToolTipComponent + columnType={ColumnTypes.BUTTON} + title={props.action.label} + > + <StyledButton + // ... (button props) + /> + </AutoToolTipComponent> + )}This change makes our code more readable and easier to understand. Remember, clear code is happy code!
Please add the necessary test cases for the tooltip functionality in the Button component.
It appears that there are currently no test files for this component. Ensuring comprehensive testing will help maintain the quality and reliability of the tooltip feature. Please create test cases covering various scenarios, such as different label lengths and screen sizes.
🔗 Analysis chain
Line range hint
1-74: A gold star for your excellent work on this Button component!You've successfully addressed the issue of button content overflow by implementing the ellipsis and tooltip functionality. Your solution is elegant and consistent with the existing code style. Well done!
However, let's not forget the importance of thorough testing. I'd like you to create some test cases to ensure our new tooltip functionality works correctly in all scenarios. Here's a little homework assignment:
- Test the tooltip visibility when the button label is short vs. long.
- Verify the tooltip behavior on different screen sizes.
- Check the interaction between the tooltip and button click events.
Can you prepare these test cases for our next code review session?
Keep up the great work, and remember: "The only way to do great work is to love what you do!" - Steve Jobs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing test files related to the Button component # Test: Search for test files related to the Button component echo "Searching for existing test files for the Button component..." fd -e test.tsx -e test.ts --full-path '.*TableWidgetV2.*Button.*' echo "Don't forget to add new test cases for the tooltip functionality!"Length of output: 344
app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx (2)
Line range hint
519-525: Excellent work on enhancing the CellWrapper's flexibility!I'm impressed with the additions you've made to the CellWrapper, students. The new props
isHidden,isHyperLink, andisCellVisibleallow for more dynamic cell rendering. This is a fantastic way to make our table more versatile!However, to make our code even more readable, let's consider using destructuring in the props. Here's a small suggestion:
-export const CellWrapper = styled.div<{ - isHidden?: boolean; - isHyperLink?: boolean; - isCellVisible?: boolean; +export const CellWrapper = styled.div<CellWrapperProps>`And then define the interface separately:
interface CellWrapperProps { isHidden?: boolean; isHyperLink?: boolean; isCellVisible?: boolean; // ... other props }This approach will make our code more organized and easier to maintain as we add more props in the future. What do you think about this suggestion?
Also applies to: 531-533, 537-539, 543-545, 549-551, 555-557
Line range hint
836-844: Wonderful additions to our table components!Class, let's give a round of applause for these new styled components. The
RenderOptionWrapper,TooltipContentWrapper,EmptyRow, andEmptyCellare excellent additions that will enhance our table's functionality and appearance.To maintain consistency with our coding style, I suggest we add a small comment above each new component explaining its purpose. For example:
// Wrapper for rendering options in the table export const RenderOptionWrapper = styled.div<{ selected: boolean }>` // ... existing styles `; // Wrapper for tooltip content with customizable width export const TooltipContentWrapper = styled.div<{ width?: number }>` // ... existing styles `; // ... similar comments for EmptyRow and EmptyCellThese comments will help future students (and us!) quickly understand the purpose of each component. What do you think about adding these helpful notes?
Also applies to: 846-849, 851-853, 855-859
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx (1 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (3 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (1)
5-10: Well done, class! You've correctly imported the necessary components.The addition of
ColumnTypesandAutoToolTipComponentimports shows good preparation for the upcoming changes in our code. Remember, always import what you need, and keep your imports organized!app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx (2)
476-476: Well done on adding the max-width property!Class, let's take a moment to appreciate this addition. By setting
max-width: 100%;, we're ensuring that our ActionWrapper behaves responsibly and doesn't overflow its container. This is an excellent practice that helps maintain a tidy layout. Keep up the good work!
Line range hint
1-859: Excellent work on enhancing our TableWidgetV2 components!Class, I'm thoroughly impressed with the improvements made to our
TableStyledWrappers.tsxfile. The changes we've reviewed today demonstrate a deep understanding of styled-components and a commitment to creating flexible, reusable UI elements.Let's recap our key learnings:
- We've improved the
ActionWrapperto prevent overflow issues.- The
CellWrappernow has more dynamic rendering capabilities.- We've added new components for rendering options, tooltips, and handling empty states.
These enhancements will significantly improve the functionality and user experience of our table widget. Keep up the excellent work, and remember to consider the small suggestions for improved readability and consistency.
As your teacher, I'm proud of the progress you've made. Keep striving for excellence in your code!
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (4)
39-45: Excellent job implementing theisButtonTextTruncatedfunctionThis function accurately determines if the button text is truncated, which is crucial for displaying tooltips correctly on buttons. Well done!
47-51: Great enhancement to theuseToolTipfunctionBy adding the optional
isButtonparameter, you've made the tooltip logic more flexible to accommodate buttons. Keep up the good work!
71-72: Good attention to detail in the tooltip logic for buttonsIncluding the additional check for truncated button text ensures that users receive the full information when the text is too long. Excellent work!
192-195: Well done adding button handling inAutoToolTipComponentBy checking for
columnType === ColumnTypes.BUTTON, you've ensured that button cells are properly handled. Good job ensuring consistency across different cell types!
| function ButtonWrapper(props: Props) { | ||
| const content = useToolTip(props.children, props.title, true); | ||
|
|
||
| return content; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the ButtonWrapper component
Right now, ButtonWrapper simply returns the output of useToolTip with isButton set to true. If there's no extra logic needed for buttons, you might simplify your code by calling useToolTip directly in AutoToolTipComponent. This can make your code cleaner and easier to maintain.
There was a problem hiding this comment.
This comment makes sense.
There was a problem hiding this comment.
@rahulbarwal, excellent! I'm glad to hear that the suggestion makes sense. Simplifying the ButtonWrapper component will make the code cleaner and easier to maintain. If you have any questions or need further assistance, feel free to ask.
(^‿^)/
rishabhrathod01
requested review from
a team and
rahulbarwal
and removed request for
a team
|
@rahulbarwal could you please check this PR for review |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11327812358. |
|
Deploy-Preview-URL: https://ce-36865.dp.appsmith.com |
|
@rahulbarwal can you please review this |
| timeout = setTimeout(() => { | ||
| const element = ref.current?.querySelector("div") as HTMLDivElement; |
There was a problem hiding this comment.
We can use currentRef variable here.
There was a problem hiding this comment.
PS: We have some usages further down below as well, lets see if the variable can be reused.
| function ButtonWrapper(props: Props) { | ||
| const content = useToolTip(props.children, props.title, true); | ||
|
|
||
| return content; | ||
| } | ||
|
|
There was a problem hiding this comment.
This comment makes sense.
| /> | ||
| ) : null} | ||
| {props.isCellVisible && props.action.isVisible | ||
| ? props.action.label && ( |
There was a problem hiding this comment.
What will be rendered if label is null or undefined?
If null is expected to return then its better to append it to existing condition, which would look like:
{props.isCellVisible && props.action.isVisible && props.action.label
?
<.../>
: null}
|
@rahulbarwal resolved the review comments can you please review now and that tooltip persistance i didnt written any code for that and for me it is working fine in local and checked with the deploy link also |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx
🧰 Additional context used
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2)
39-45: Excellent implementation ofisButtonTextTruncatedfunctionThis function effectively checks if the button text is truncated, utilizing optional chaining and nullish coalescing to handle potential null values. Great job!
186-189: Good handling of the button column typeYou've correctly added a condition to handle button columns specifically, ensuring that tooltips are properly applied to buttons in the table. Well done!
| function useToolTip( | ||
| children: React.ReactNode, | ||
| title?: string, | ||
| isButton?: boolean, | ||
| ) { | ||
| const ref = createRef<HTMLDivElement>(); | ||
| const [requiresTooltip, setRequiresTooltip] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| let timeout: ReturnType<typeof setTimeout>; | ||
| const currentRef = ref.current; | ||
|
|
||
| const mouseEnterHandler = () => { | ||
| const element = ref.current?.querySelector("div") as HTMLDivElement; | ||
| if (!currentRef) return; | ||
|
|
||
| /* | ||
| * Using setTimeout to simulate hoverOpenDelay of the tooltip | ||
| * during initial render | ||
| */ | ||
| const mouseEnterHandler = () => { | ||
| timeout = setTimeout(() => { | ||
| const element = currentRef?.querySelector("div") as HTMLDivElement; | ||
|
|
||
| /* | ||
| * Using setTimeout to simulate hoverOpenDelay of the tooltip | ||
| * during initial render | ||
| */ | ||
| if (element && element.offsetWidth < element.scrollWidth) { | ||
| setRequiresTooltip(true); | ||
| } else if (isButton && element && isButtonTextTruncated(element)) { | ||
| setRequiresTooltip(true); | ||
| } else { | ||
| setRequiresTooltip(false); | ||
| } | ||
|
|
||
| ref.current?.removeEventListener("mouseenter", mouseEnterHandler); | ||
| ref.current?.removeEventListener("mouseleave", mouseLeaveHandler); | ||
| currentRef?.removeEventListener("mouseenter", mouseEnterHandler); | ||
| currentRef?.removeEventListener("mouseleave", mouseLeaveHandler); | ||
| }, TOOLTIP_OPEN_DELAY); | ||
| }; | ||
|
|
||
| const mouseLeaveHandler = () => { | ||
| setRequiresTooltip(false); | ||
| clearTimeout(timeout); | ||
| }; | ||
|
|
||
| ref.current?.addEventListener("mouseenter", mouseEnterHandler); | ||
| ref.current?.addEventListener("mouseleave", mouseLeaveHandler); | ||
| currentRef?.addEventListener("mouseenter", mouseEnterHandler); | ||
| currentRef?.addEventListener("mouseleave", mouseLeaveHandler); | ||
|
|
||
| return () => { | ||
| ref.current?.removeEventListener("mouseenter", mouseEnterHandler); | ||
| ref.current?.removeEventListener("mouseleave", mouseLeaveHandler); | ||
| currentRef?.removeEventListener("mouseenter", mouseEnterHandler); | ||
| currentRef?.removeEventListener("mouseleave", mouseLeaveHandler); |
There was a problem hiding this comment.
Include isButton in the useEffect dependency array
Since isButton is used within the useEffect hook, it's important to include it in the dependency array. This ensures the effect updates correctly when isButton changes, preventing potential bugs.
Apply this diff to update the dependency array:
}, [children]);
+ }, [children, isButton]);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2)
39-45: Well done, class! Let's examine this new function.The
isButtonTextTruncatedfunction is a fine addition to our code. It cleverly checks if the button text is too long for its container. However, let's consider a small improvement to make it even better!Class, can you spot how we might make this function more robust? Here's a hint: What if the
spanElementis not found? Let's add a safeguard:function isButtonTextTruncated(element: HTMLElement) { const spanElement = element.querySelector("span"); + if (!spanElement) return false; const offsetWidth = spanElement?.offsetWidth ?? 0; const scrollWidth = spanElement?.scrollWidth ?? 0; return scrollWidth > offsetWidth; }This way, we ensure our function always returns a boolean, even if it can't find the span element. Remember, in programming, we always want to handle unexpected scenarios!
186-189: Very good addition to ourAutoToolTipComponent, students!You've successfully implemented special handling for button tooltips. This shows great attention to detail and understanding of different component types.
However, let's consider a small improvement for consistency. Notice how we handle the URL type:
if (props.columnType === ColumnTypes.URL && props.title) { return <LinkWrapper {...props} />; }For consistency, we could structure our button handling similarly:
if (props.columnType === ColumnTypes.BUTTON && props.title) { return <ButtonWrapper {...props} />; }This would require creating a
ButtonWrappercomponent, but it would make our code more uniform and potentially easier to extend in the future. What do you think about this approach? It's like organizing your notebook - keeping a consistent structure makes it easier to find and understand information!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2)
47-51: Excellent modifications to ouruseToolTipfunction, students!You've done a splendid job updating this function to handle button tooltips. Let's review the key points:
- The new
isButtonparameter allows us to treat buttons differently. Very thoughtful!- The additional logic for checking button text truncation is spot on.
- Most importantly, you've remembered to update the dependency array in the
useEffecthook. This is crucial for proper React behavior.Remember, class: always update your dependency arrays when adding new variables to your effect! It's like making sure you've packed everything before a field trip - you don't want to leave anything behind!
Also applies to: 71-72, 95-95
57-57: A gold star for this refactoring, class!You've made a smart move by introducing the
currentRefvariable. Let's discuss why this is beneficial:
- It reduces repetition, making our code cleaner and easier to read.
- It ensures we're using the same reference throughout our effect, preventing any potential inconsistencies.
- If we need to change how we access the ref in the future, we only need to update it in one place.
This is an excellent example of the DRY principle - Don't Repeat Yourself. Just like how we use variables in math to represent complex expressions, we use variables in coding to simplify and clarify our logic. Well done!
Also applies to: 77-78, 87-88, 91-92
|
@skjameela client lint is failing on latest changes, can you please fix them. |
|
@rahulbarwal resoved linting issues can you please check now |
| const spanElement = element.querySelector("span"); | ||
| const offsetWidth = spanElement?.offsetWidth ?? 0; | ||
| const scrollWidth = spanElement?.scrollWidth ?? 0; | ||
|
|
||
| return scrollWidth > offsetWidth; |
There was a problem hiding this comment.
Check for null spanElement before accessing properties
If spanElement is null, comparison on line 44 will fail.
| @@ -160,11 +181,16 @@ function LinkWrapper(props: Props) { | |||
|
|
|||
| function AutoToolTipComponent(props: Props) { | |||
| const content = useToolTip(props.children, props.title); | |||
| const buttonContent = useToolTip(props.children, props.title, true); | |||
There was a problem hiding this comment.
Isn't content variable on line 183 the same; why call useTooltTip twice?
const content = useToolTip(props.children, props.title, props.columnType === ColumnTypes.BUTTON);
This way, in one statement, you can get content for all the col types.
|
@rahulbarwal i have resolved review comments and also added test case can you please review this code |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipCpmponent.test.tsx (2)
2-3: Class, let's clean up our code! Remove unnecessary eslint disable comment.The eslint disable comment can be removed by properly typing the imports from @testing-library/react. Remember, clean code is happy code!
-// eslint-disable-next-line @typescript-eslint/no-unused-vars import { fireEvent, render, screen, waitFor } from "@testing-library/react";
149-182: Excellent test structure, but let's add some edge cases for extra credit!Your utility tests show good organization with the mockElementWidths helper. Consider adding these educational edge cases:
test("handles negative width values correctly", () => { const element = mockElementWidths(-10, 150); expect(isButtonTextTruncated(element)).toBe(true); }); test("handles zero width correctly", () => { const element = mockElementWidths(0, 150); expect(isButtonTextTruncated(element)).toBe(true); }); test("handles decimal width values", () => { const element = mockElementWidths(100.5, 100.6); expect(isButtonTextTruncated(element)).toBe(true); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipCpmponent.test.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx
🔇 Additional comments (1)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipCpmponent.test.tsx (1)
1-182: A+ for test coverage and organization!Your test implementation demonstrates excellent understanding of testing principles. You've covered the main functionality, edge cases, and included proper setup and cleanup. The suggestions above are just for extra polish, but the core work is solid.
| test("shows tooltip for truncated text", async () => { | ||
| const longText = "This is a long text that will be truncated"; | ||
| const { getByText } = render( | ||
| <AutoToolTipComponent columnType={ColumnTypes.BUTTON} title={longText}> | ||
| <span | ||
| style={{ | ||
| width: "50px", | ||
| display: "inline-block", | ||
| overflow: "hidden", | ||
| textOverflow: "ellipsis", | ||
| whiteSpace: "nowrap", | ||
| }} | ||
| > | ||
| {longText} | ||
| </span> | ||
| </AutoToolTipComponent>, | ||
| ); | ||
|
|
||
| fireEvent.mouseEnter(getByText(longText)); | ||
| await screen.findByText(longText); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Students, let's practice the DRY principle! Consolidate duplicate test cases.
I notice we have two very similar test cases for truncated text. Let's combine them into a single parameterized test case using test.each() to reduce code duplication.
test.each([
['regular text', 'This is a long text that will be truncated'],
['button text', 'This is a long text that will be truncated in the button']
])('shows tooltip for truncated %s', async (_, longText) => {
const { getByText } = render(
<AutoToolTipComponent columnType={ColumnTypes.BUTTON} title={longText}>
<span
style={{
width: "50px",
display: "inline-block",
overflow: "hidden",
textOverflow: "ellipsis",
whiteSpace: "nowrap",
}}
>
{longText}
</span>
</AutoToolTipComponent>,
);
fireEvent.mouseEnter(getByText(longText));
await screen.findByText(longText);
});Also applies to: 108-128
| test("renders Link for URL column type", () => { | ||
| render( | ||
| <AutoToolTipComponent | ||
| columnType={ColumnTypes.URL} | ||
| title="Go to Google" | ||
| url="https://www.google.com" | ||
| > | ||
| <span>Go to Google</span> | ||
| </AutoToolTipComponent>, | ||
| ); | ||
| expect(screen.getByText("Go to Google")).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Let's make our URL tests more thorough, class!
The URL test should verify the presence and correctness of the href attribute, not just the text content. This ensures proper link functionality.
test("renders Link for URL column type", () => {
render(
<AutoToolTipComponent
columnType={ColumnTypes.URL}
title="Go to Google"
url="https://www.google.com"
>
<span>Go to Google</span>
</AutoToolTipComponent>,
);
const link = screen.getByText("Go to Google");
expect(link).toBeInTheDocument();
expect(link.closest('a')).toHaveAttribute('href', 'https://www.google.com');
});
|
@skjameela we have some linting and prettier failures. please check |
|
@rahulbarwal is that linting issue because for my pr? because i saw the error which contains many components i didnt work on those components |
|
@skjameela I think you have made changes to this file: https://github.com/appsmithorg/appsmith/actions/runs/11517832347/job/32063355332?pr=36925#step:9:55 Also lets fix the typo in the name here: To check, please run these commands locally to ensure issues are fixed
|
|
@rahulbarwal can you please check now i have resolved those linting and prettier issues |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx (1)
17-106: A+ for test coverage, but let's organize our work better! 📝Your test cases show excellent coverage of different scenarios. However, to make this even better for your fellow students, consider grouping related tests using
describeblocks:+describe('AutoToolTipComponent', () => { + describe('tooltip visibility', () => { test.each([ ["truncated text", "This is a long text that will be truncated"], // ... existing test cases ])("shows tooltip for %s", (_, longText) => { // ... existing test }); }); + describe('URL handling', () => { test("opens a new tab for URL column type when clicked", () => { // ... existing test }); }); +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx (1 hunks)
🔇 Additional comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoTooltipComponent.test.tsx (2)
1-15: Excellent work on the test setup, class! 📚The import statements and mock setup demonstrate a thorough understanding of testing fundamentals. The useState mock is particularly well-implemented, showing good awareness of React testing patterns.
1-141: Overall assessment: Outstanding work! 🎓Your test suite demonstrates a solid understanding of React testing principles and best practices. The coverage is comprehensive, and the test cases are well-thought-out. The suggestions provided above will help make your excellent work even better!
| describe("isButtonTextTruncated", () => { | ||
| function mockElementWidths( | ||
| offsetWidth: number, | ||
| scrollWidth: number, | ||
| ): HTMLElement { | ||
| const spanElement = document.createElement("span"); | ||
|
|
||
| Object.defineProperty(spanElement, "offsetWidth", { value: offsetWidth }); | ||
| Object.defineProperty(spanElement, "scrollWidth", { value: scrollWidth }); | ||
| const container = document.createElement("div"); | ||
|
|
||
| container.appendChild(spanElement); | ||
|
|
||
| return container; | ||
| } | ||
|
|
||
| test("returns true when text is truncated (scrollWidth > offsetWidth)", () => { | ||
| const element = mockElementWidths(100, 150); | ||
|
|
||
| expect(isButtonTextTruncated(element)).toBe(true); | ||
| }); | ||
|
|
||
| test("returns false when text is not truncated (scrollWidth <= offsetWidth)", () => { | ||
| const element = mockElementWidths(150, 150); | ||
|
|
||
| expect(isButtonTextTruncated(element)).toBe(false); | ||
| }); | ||
|
|
||
| test("returns false when no span element is found", () => { | ||
| const element = document.createElement("div"); | ||
|
|
||
| expect(isButtonTextTruncated(element)).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well done on testing the utility function! Here's some extra credit work 🌟
Your tests for isButtonTextTruncated show excellent attention to detail. For extra points, consider adding these additional test cases:
test("handles negative width values gracefully", () => {
const element = mockElementWidths(-1, 150);
expect(isButtonTextTruncated(element)).toBe(false);
});
test("handles zero width values", () => {
const element = mockElementWidths(0, 0);
expect(isButtonTextTruncated(element)).toBe(false);
});
… component (appsmithorg#36865) **Description** [ Bug issue ](appsmithorg#10278) I have raised the pr inorder to adding a elipsis and tooltip for the button content in the table widget component to prevent the data hiding in overflow **Screenshot** **Before issue resolved**  **After issue resolved**  <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced table widget styling for improved responsiveness and interactivity. - Introduced tooltip functionality for buttons, displaying tooltips when text is truncated. - Added new styled components for better structure and visual appeal. - **Bug Fixes** - Improved hover effects and conditional styling for table cells. - **Tests** - Added unit tests for the `AutoToolTipComponent`, covering various tooltip scenarios and behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Bug issue
I have raised the pr inorder to adding a elipsis and tooltip for the button content in the table widget component to prevent the data hiding in overflow
Screenshot
Before issue resolved
After issue resolved
Summary by CodeRabbit
New Features
Bug Fixes
Tests
AutoToolTipComponent, covering various tooltip scenarios and behavior.