Create skeleton card component#70
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSGLSkeleton: a thin wrapper around MUI Skeleton that omits ChangesSkeleton Components Library
SGLSelect Interaction Tests
Sequence Diagram(s)sequenceDiagram
participant SGLSkeleton
participant Theme
participant skeletonStyles
participant MuiSkeleton
SGLSkeleton->>Theme: access theme in sx callback
SGLSkeleton->>skeletonStyles: call skeletonStyles(theme)
skeletonStyles-->>SGLSkeleton: return style object
SGLSkeleton->>MuiSkeleton: render with sx = merged(style object, provided style) and animation='wave'
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/UI/Skeleton/SGLContentCardSkeleton.css (1)
142-143: ⚡ Quick winReplace deprecated
word-break: break-word.Use modern wrapping behavior (
overflow-wrap) and avoid deprecatedword-breakkeyword.Proposed fix
.image-description { position: relative; @@ - word-break: break-word; overflow-wrap: anywhere; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/UI/Skeleton/SGLContentCardSkeleton.css` around lines 142 - 143, The CSS rule in SGLContentCardSkeleton.css currently uses the deprecated declaration "word-break: break-word"; remove that line and rely on "overflow-wrap: anywhere" (or add "word-break: break-all" only if intentional hard breaks are needed) so update the selector containing "word-break: break-word" to delete that property and keep "overflow-wrap: anywhere" as the modern wrapping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/UI/Skeleton/SGLContentCardSkeleton.css`:
- Around line 92-101: The CSS for .sgl-image-wrapper hardcodes height: 180px
which prevents the component from honoring the imageHeight prop; remove the
fixed height declaration from .sgl-image-wrapper and either rely on inline
styles set by the component using the imageHeight prop or add a CSS variable
(e.g., height: var(--sgl-image-height, auto)) that the component can set from
imageHeight, ensuring the wrapper's height becomes configurable at runtime.
- Around line 51-54: The CSS declaration in the .sgl-text-wrapper rule uses
invalid quoted value width: 'fit-content'; which gets ignored; update the
.sgl-text-wrapper rule to use a valid value (remove the quotes so it reads
fit-content) or use an appropriate alternative (e.g., display: inline-block or
width: max-content) to achieve the intended sizing behavior for the element.
In `@src/components/UI/Skeleton/SGLSkeleton.tsx`:
- Around line 35-41: In SGLSkeleton update the component to avoid spreading the
incoming sx (which may be an array or function) into an object; instead build a
baseSx object containing the conditional fontSize (from variant/textSize),
bgcolor (skeletonColor), borderRadius (radius), and opacity, and pass the prop
as an array composition like sx={[baseSx, sx]} so MUI can handle
object/array/function SxProps correctly; modify the code around the sx
assignment in SGLSkeleton to create baseSx and supply sx as an array rather than
using ...sx.
---
Nitpick comments:
In `@src/components/UI/Skeleton/SGLContentCardSkeleton.css`:
- Around line 142-143: The CSS rule in SGLContentCardSkeleton.css currently uses
the deprecated declaration "word-break: break-word"; remove that line and rely
on "overflow-wrap: anywhere" (or add "word-break: break-all" only if intentional
hard breaks are needed) so update the selector containing "word-break:
break-word" to delete that property and keep "overflow-wrap: anywhere" as the
modern wrapping behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cad883a-36b0-44b4-a597-a96d5a09a486
📒 Files selected for processing (3)
src/components/UI/Skeleton/SGLContentCardSkeleton.csssrc/components/UI/Skeleton/SGLContentCardSkeleton.tsxsrc/components/UI/Skeleton/SGLSkeleton.tsx
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
We are implementing our UI components in the project based on material ui (MUI) library and they already got ready to work skeleton that we can just wrap, check the docs :
https://mui.com/material-ui/react-skeleton/
The component should be similar in props to any other component under the ui folder, SGLCard for example is a simple one that you can take reference from
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/UI/Skeleton/SGLSkeleton.tsx`:
- Around line 1-3: Normalize the file's line endings from CRLF to LF for
src/components/UI/Skeleton/SGLSkeleton.tsx (including the import block
referencing Skeleton and MuiSkeletonProps) and any other affected lines (e.g.,
the occurrence at line 7), then re-run the project's formatter/prettier so the
file is saved with repository-preferred LF endings and formatting checks pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 874b4398-eeb0-4054-9a90-13f0d5f15756
📒 Files selected for processing (1)
src/components/UI/Skeleton/SGLSkeleton.tsx
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
left some comments, also do you see linter errors when you run your code locally?
| return ( | ||
| <Skeleton | ||
| sx={{ | ||
| bgcolor: skeletonColor, |
There was a problem hiding this comment.
bgcolor is MUI prop, we dont want do to it, we want to create css object like we we in the rest of the components
| } | ||
|
|
||
| export const SGLSkeleton = ({ | ||
| skeletonColor = '#8F3DFF', |
There was a problem hiding this comment.
No hardcoded colors, bring theme from the theme object in theme.ts
| interface SGLSkeletonProps | ||
| extends Omit<MuiSkeletonProps, 'sx'> { | ||
| skeletonColor?: string | ||
| sx?: SxProps<Theme> |
There was a problem hiding this comment.
not sx, the component should accept styles object, check how we are doing it in other ui components
It worked for me, hope the new version works for you too. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/UI/Skeleton/SGLSkeleton.tsx`:
- Around line 19-22: The component SGLSkeleton currently spreads the incoming
style prop into the MUI sx object (sx={(theme)=>({...skeletonStyles(theme),
...style})}), which causes numeric values to be interpreted by the MUI system;
remove spreading style into sx and instead pass the style prop through as a
plain React inline style on the root element (e.g., use style={style}), keeping
skeletonStyles/theme-derived values in sx and merging only the incoming sx prop
(not style). Ensure the prop type remains style?: CSSProperties and update any
usage in SGLSkeleton where sx is composed (refer to skeletonStyles and the
existing sx merge) so style is applied as an ordinary DOM style, not part of sx.
In `@src/components/UI/Skeleton/styles.ts`:
- Line 12: The file src/components/UI/Skeleton/styles.ts is missing a trailing
newline which breaks Prettier; open styles.ts and ensure the file ends with a
single newline character (add a '\n' at EOF), save the file so the final closing
brace is followed by a newline, then re-run Prettier/commit the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d987ac94-96d2-4c65-83ff-70deef110f93
📒 Files selected for processing (2)
src/components/UI/Skeleton/SGLSkeleton.tsxsrc/components/UI/Skeleton/styles.ts
| sx={(theme) => ({ | ||
| ...skeletonStyles(theme), | ||
| ...style, | ||
| })} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In MUI v7, how does the sxprop interpret numeric values for properties like margin, padding, borderRadius, width, and height, and how does that differ from passing the same values through the Reactstyle prop?
💡 Result:
In MUI v7, the sx prop acts as a theme-aware interface that interprets numeric values differently depending on the specific CSS property [1]. Unlike the standard React style prop, which passes values directly to the browser as inline CSS styles (often requiring explicit units like 'px' or 'rem'), the sx prop maps numbers to theme-defined values or specific transformation functions [1]. Key behaviors for numeric values in the sx prop include: 1. Spacing (margin, padding): Numeric values are multiplied by the theme.spacing value (default is 8px) [1]. For example, sx={{ m: 2 }} results in a margin of 16px. 2. Sizing (width, height, minWidth, minHeight, maxWidth, maxHeight): These properties use a custom transform function [1]. If the numeric value is between (0, 1], it is converted to a percentage (e.g., sx={{ width: 0.5 }} becomes width: 50%). If the value is greater than 1, it is treated as a pixel value (e.g., sx={{ width: 20 }} becomes width: 20px) [1]. 3. borderRadius: Numeric values are multiplied by the theme.shape.borderRadius value (default is 4px) [1]. 4. Border: Numeric values are interpreted as pixel widths for a solid black border (e.g., sx={{ border: 1 }} equates to border: 1px solid black) [1]. In contrast, the React style prop performs no such transformations [2]. Passing a number to style (e.g., style={{ margin: 2 }}) is generally invalid for CSS properties that require units, as React will not automatically append 'px' to most numeric values, resulting in the style being ignored by the browser [2]. The sx prop provides a more concise, theme-integrated approach, whereas the style prop remains a standard, direct way to apply inline styles without MUI-specific logic or theme awareness [2].
Citations:
- 1: https://v7.mui.com/system/getting-started/the-sx-prop/
- 2: https://blog.stackademic.com/mui-when-to-use-the-sx-prop-vs-style-prop-for-styling-in-react-and-next-js-7997f2aca588
Fix style prop contract: don’t merge style into MUI sx
style?: CSSProperties implies React inline-style semantics, but spreading style into sx makes numeric values follow MUI system transforms (e.g., spacing via theme.spacing, borderRadius scaling via theme.shape.borderRadius, special transforms for width/height). That changes how callers’ values are interpreted versus a real React style prop.
Proposed fix
<Skeleton
- sx={(theme) => ({
- ...skeletonStyles(theme),
- ...style,
- })}
+ sx={(theme) => skeletonStyles(theme)}
+ style={style}
animation={animation}
{...props}
/>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/UI/Skeleton/SGLSkeleton.tsx` around lines 19 - 22, The
component SGLSkeleton currently spreads the incoming style prop into the MUI sx
object (sx={(theme)=>({...skeletonStyles(theme), ...style})}), which causes
numeric values to be interpreted by the MUI system; remove spreading style into
sx and instead pass the style prop through as a plain React inline style on the
root element (e.g., use style={style}), keeping skeletonStyles/theme-derived
values in sx and merging only the incoming sx prop (not style). Ensure the prop
type remains style?: CSSProperties and update any usage in SGLSkeleton where sx
is composed (refer to skeletonStyles and the existing sx merge) so style is
applied as an ordinary DOM style, not part of sx.
|
@rotemergty5 Notice that the ESLint auto check failed. Please Fix it |
There was a problem hiding this comment.
@rotemergty5 The code works locally but you have prettier error, run prettier locally and push them
|
@Tamir198 I fixed all the review comments related to the Skeleton component. ESLint, Prettier, build and tests are passing successfully. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/UI/Select/SGLselect.test.tsx`:
- Line 45: The assertion using expect(select).not.toHaveTextContent('Banana') is
too weak; update the test in SGLselect.test.tsx to explicitly assert the
empty/unselected state for the select element (variable select) — for example
replace the negative check with a direct check like
expect(select).toHaveTextContent('') or assert the value/placeholder by using
expect(select).toHaveValue('') or
expect(screen.queryByText('Banana')).toBeNull(), ensuring the test verifies the
select is truly empty/unselected rather than merely not containing 'Banana'.
- Around line 25-29: Convert the flaky synchronous option lookups in
SGLselect.test.tsx to async queries and stronger assertions: make the test
functions async, replace fireEvent.mouseDown(select) +
screen.getByText('Banana') with await screen.findByRole('option', { name:
'Banana' }) (or await screen.findByText('Banana')) before interacting, then
await user selection (or fireEvent.click on the awaited option); and in the
Escape test assert that none of the options are present (e.g., await waitFor(()
=> expect(screen.queryByRole('option', { name: /Banana|Apple|.../
})).toBeNull()) or assert the visible selected value is empty rather than only
asserting “Banana” is not shown. Ensure you update references to
screen.getByText, fireEvent.mouseDown/select click code, and the Escape
assertion in SGLselect.test.tsx accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 789171f9-117d-480c-a400-adc7b096eb6f
📒 Files selected for processing (1)
src/components/UI/Select/SGLselect.test.tsx
| fireEvent.mouseDown(select) | ||
|
|
||
| const option = screen.getByText('Banana') | ||
| fireEvent.click(option) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File exists? =="
ls -la src/components/UI/Select/SGLselect.test.tsx
echo "== Show test file around referenced lines =="
sed -n '1,120p' src/components/UI/Select/SGLselect.test.tsx | nl -ba
echo "== Show SGLSelect implementation and related files =="
fd -t f -a "SGLselect|SGLSelect" src/components/UI/Select || true
# list plausible implementation files
fd -t f "SGLselect*" src/components/UI/Select || true
fd -t f "SGLSelect*" src/components/UI/Select || true
# If we find the main implementation file(s), print them (limited)
for f in $(fd -t f -i "SGL.*select.*" src/components/UI/Select); do
echo "---- $f (head 200) ----"
sed -n '1,200p' "$f" | nl -ba
done | head -n 400Repository: ColmanDevClubORG/Sagol360Management
Length of output: 297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show test file around lines 1-110 with line numbers =="
sed -n '1,110p' src/components/UI/Select/SGLselect.test.tsx | cat -n
echo "== Locate likely implementation files =="
ls -ლა src/components/UI/Select || true
fd -t f -a "SGLselect|SGLSelect" src/components/UI/Select 2>/dev/null || true
fd -t f "SGLselect*" src/components/UI/Select 2>/dev/null || true
fd -t f "SGLSelect*" src/components/UI/Select 2>/dev/null || true
echo "== Search repo for component export/usage of SGLSelect =="
rg -n "SGLSelect|SGLselect" src/components/UI/Select -S || true
rg -n "function SGLSelect|const SGLSelect|export .*SGLSelect" src -S || trueRepository: ColmanDevClubORG/Sagol360Management
Length of output: 3511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SGLSelect.tsx =="
sed -n '1,200p' src/components/UI/Select/SGLSelect.tsx | cat -n
echo "== Search for MenuItem / Select usage in that file =="
rg -n "MenuItem|Select|renderValue|MenuProps|disablePortal|portal|Paper|Popover|PopoverProps|role=" src/components/UI/Select/SGLSelect.tsx
echo "== Check if any global MUI test setup sets disablePortal or RTL config =="
rg -n "disablePortal|`@testing-library/react`|configure|setupFiles|testEnvironment|jest-dom" -S . || trueRepository: ColmanDevClubORG/Sagol360Management
Length of output: 3560
🌐 Web query:
MUI v5 Select MenuItem role option menuitem aria roles listbox option
💡 Result:
In MUI v5, you typically do not need to manually add ARIA roles like listbox/option/menuitem on MenuItem when using the standard (non-native) pattern. The Select component is designed to be a drop-down that is interchangeable with a native element, and it manages the accessibility/labeling requirements for the overall control. The items you pass as children to should be MenuItem components (when native={false}); these are the “options” of the listbox UI that the Select renders. [1][2] What you should focus on (grounded in MUI docs) 1) Ensure the Select has an accessible name. - MUI’s React Select docs state that “Select always needs an accessible name” and this can come from an associated visible label (InputLabel linked via labelId) or from adding an aria-label to the input element props (inputProps). [3][4] - Correct label wiring example (InputLabel with matching labelId and MenuItem children): [3][5] 2) Don’t confuse MenuItem for ARIA “menuitem”. - MDN’s role=menuitem applies to items inside elements with role=menu or menubar (a menu pattern), not the select/listbox pattern. [6] - For select-like widgets, the relevant structural roles are listbox/option (rather than menu/menuitem). MDN describes listbox as a container whose children have role=option. [7] - As a result, setting MenuItem role="menuitem" is generally the wrong pattern for Select. So what is the “correct roles listbox option” mapping? - Use the Select as a whole (MUI does the right thing) and provide labeled options via MenuItem children. - If you were building from scratch, a listbox would be role="listbox" and each option role="option" (with aria-selected on options). [7] - But in MUI v5 Select, you usually leave those ARIA roles to the component rather than applying them to MenuItem manually. Instead, ensure the control is properly labeled and that MenuItem is used as direct descendants (non-native case). [1][2] Practical checklist for accessibility with MUI v5 Select + MenuItem - Non-native: and MenuItem as direct descendants. [1] - Label the Select (recommended): - ... - ... - The accessible name requirement is explicitly called out by MUI docs. [3][4] - Then convey item state/content via the visible MenuItem text (and optionally iconography). MUI’s docs show icon-based selection state inside the list. [3] If you need checkbox/switch states inside options - MUI’s Select/ MenuItem pattern is fundamentally a single/multiple selection control; putting interactive controls (checkbox/switch) inside MenuItem doesn’t automatically make them announced as “checkbox checked/unchecked” unless you also provide appropriate accessible name/state semantics for those inner controls. - The specific “role mapping” still should follow the select/listbox pattern (not menu/menuitem), and you’ll likely need to ensure the inner control has accessible text/state (e.g., via aria-checked and an accessible name on the checkbox/switch element), while still keeping the overall Select labeling correct. (General ARIA guidance: menuitem applies to menus, listbox/options apply to selects.) [6][7] In short - Don’t manually force MenuItem role="menuitem" for MUI Select; menuitem is for role=menu patterns. [6] - For a select-like UI, the underlying pattern is listbox/options. [7] - With MUI v5, you should instead use MenuItem children correctly and ensure the Select has an accessible name via InputLabel/labelId (or aria-label). [1][3][4]
Citations:
- 1: https://mui.com/material-ui/api/select/
- 2: https://next.mui.com/material-ui/api/select/
- 3: https://mui.com/material-ui/react-select/
- 4: https://mui.com/material-ui/react-select.md
- 5: https://github.com/mui/material-ui/blob/next/docs/data/material/components/selects/selects.md
- 6: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/menuitem_role
- 7: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/listbox_role
🌐 Web query:
MUI Select MenuItem role option listbox accessibility role option
💡 Result:
In MUI (Material-UI), the Select component is designed to align with the WAI-ARIA Combobox pattern [1][2]. The primary component itself serves as a combobox, and the dropdown container that holds the selectable items functions as a listbox [1][3]. Key accessibility roles and attributes include: 1. Combobox: The main trigger element (the Select component itself) functions as a combobox [1][3]. It typically carries the role="combobox" and attributes such as aria-haspopup="listbox", aria-expanded (indicating the current state), and aria-controls (linking to the listbox ID) [1][2][3]. 2. Listbox: The popup container that appears when the Select is opened acts as the listbox, possessing the role="listbox" [3]. 3. Option: Each individual MenuItem component within the Select serves as an option, carrying the role="option" [4][3]. When an item is selected or highlighted, state attributes such as aria-selected="true" or aria-selected="false" are used to communicate its status to assistive technologies [1][5][3]. To ensure full accessibility, the Select must have an accessible name, typically provided by an associated InputLabel (linked via labelId) or an aria-label prop [6]. When using grouped options, it is essential to ensure that the combobox correctly associates with its label [7]. While MUI handles much of this semantic structure internally, developers should verify these relationships if they are building custom wrappers or overriding standard component behaviors [8]. Note that the select role itself is an abstract role in WAI-ARIA and should not be used directly on elements; the implementation relies on the specific combobox and listbox roles described above [9].
Citations:
- 1: [Select] Improve implementation to align better with ARIA documentation mui/material-ui#35586
- 2: [Select] Should have role='listbox' or 'combobox' instead of 'button' mui/material-ui#16409
- 3: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/
- 4: https://www.w3.org/TR/2021/NOTE-wai-aria-practices-1.2-20211129/examples/combobox/combobox-select-only.html
- 5: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-autocomplete-none/
- 6: https://mui.com/material-ui/react-select/
- 7: [docs][Select] Fix labeling issues in grouped select demo mui/material-ui#46722
- 8: https://dev.to/naomir/accessibility-bugs-that-were-actually-architecture-problems-1fmh
- 9: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/select_role
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vitest.setup.ts =="
cat -n vitest.setup.tsRepository: ColmanDevClubORG/Sagol360Management
Length of output: 150
Fix flaky MUI Select option lookups and strengthen the Escape assertion in SGLselect.test.tsx
- Reliability (major):
SGLSelectuses MUISelect+MenuItemwithout any portal/transition overrides, soscreen.getByText(...)right aftermouseDowncan race. Use async queries likeawait screen.findByRole('option', { name: ... })and make the testsasync. - Assertion strength (minor): The Escape test only asserts “Banana” isn’t shown; it can still pass if another option gets selected. Assert none of the provided options are rendered (or assert the displayed value is empty).
Proposed fix
- it('should select the relevant item', () => {
+ it('should select the relevant item', async () => {
@@
- fireEvent.mouseDown(select)
-
- const option = screen.getByText('Banana')
+ fireEvent.mouseDown(select)
+ const option = await screen.findByRole('option', { name: 'Banana' })
fireEvent.click(option)
@@
- it('should change the selected value when selecting a different option', () => {
+ it('should change the selected value when selecting a different option', async () => {
@@
- fireEvent.mouseDown(select)
-
- const bananaOption = screen.getByText('Banana')
+ fireEvent.mouseDown(select)
+ const bananaOption = await screen.findByRole('option', { name: 'Banana' })
fireEvent.click(bananaOption)
@@
- fireEvent.mouseDown(select)
-
- const orangeOption = screen.getByText('Orange')
+ fireEvent.mouseDown(select)
+ const orangeOption = await screen.findByRole('option', { name: 'Orange' })
fireEvent.click(orangeOption)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fireEvent.mouseDown(select) | |
| const option = screen.getByText('Banana') | |
| fireEvent.click(option) | |
| fireEvent.mouseDown(select) | |
| const option = await screen.findByRole('option', { name: 'Banana' }) | |
| fireEvent.click(option) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/UI/Select/SGLselect.test.tsx` around lines 25 - 29, Convert
the flaky synchronous option lookups in SGLselect.test.tsx to async queries and
stronger assertions: make the test functions async, replace
fireEvent.mouseDown(select) + screen.getByText('Banana') with await
screen.findByRole('option', { name: 'Banana' }) (or await
screen.findByText('Banana')) before interacting, then await user selection (or
fireEvent.click on the awaited option); and in the Escape test assert that none
of the options are present (e.g., await waitFor(() =>
expect(screen.queryByRole('option', { name: /Banana|Apple|.../ })).toBeNull())
or assert the visible selected value is empty rather than only asserting
“Banana” is not shown. Ensure you update references to screen.getByText,
fireEvent.mouseDown/select click code, and the Escape assertion in
SGLselect.test.tsx accordingly.
| fireEvent.mouseDown(select) | ||
| fireEvent.keyDown(select, { key: 'Escape' }) | ||
|
|
||
| expect(select).not.toHaveTextContent('Banana') |
There was a problem hiding this comment.
Strengthen the “opened and closed” assertion.
not.toHaveTextContent('Banana') is too weak and can pass on wrong states. Assert the expected empty/unselected state explicitly.
Proposed fix
- expect(select).not.toHaveTextContent('Banana')
+ expect(select).toHaveTextContent('')
+ expect(select).not.toHaveTextContent(/Apple|Banana|Orange/)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(select).not.toHaveTextContent('Banana') | |
| expect(select).toHaveTextContent('') | |
| expect(select).not.toHaveTextContent(/Apple|Banana|Orange/) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/UI/Select/SGLselect.test.tsx` at line 45, The assertion using
expect(select).not.toHaveTextContent('Banana') is too weak; update the test in
SGLselect.test.tsx to explicitly assert the empty/unselected state for the
select element (variable select) — for example replace the negative check with a
direct check like expect(select).toHaveTextContent('') or assert the
value/placeholder by using expect(select).toHaveValue('') or
expect(screen.queryByText('Banana')).toBeNull(), ensuring the test verifies the
select is truly empty/unselected rather than merely not containing 'Banana'.
| cleanup() | ||
| }) | ||
|
|
||
| describe('SGLSelect', () => { |
There was a problem hiding this comment.
Those changes are not related to the skeleton pr please remove them
3 participants
I built Skeleton components in React and MUI in a clean and reusable way.
I created a base SGLSkeleton component that supports customizable colors, sizes, and styling, and on top of it I built a card component that includes a profile image, title, description, image, and button.
I also added support for passing different texts, colors, and images to each card, including a default placeholder image when no image is provided.
Summary by CodeRabbit
Refactor
New Features
Tests