RI-7790: update Select Index menu styles #5303
Conversation
KrumTy
requested review from
ArtemHoruzhenko,
DimoHG,
dantovska,
pawelangelow,
pd-redis,
tptodorov and
valkirilov
as code owners
Code Coverage - Frontend unit tests
Test suite run success5468 tests passing in 703 suites. Report generated by 🧪jest coverage report action from ecb17de |
| import styled from 'styled-components' | ||
|
|
||
| export const Container = styled.div<React.HTMLAttributes<HTMLDivElement>>` | ||
| height: 36px; |
There was a problem hiding this comment.
Please use some of the theme values.
There was a problem hiding this comment.
this is the style of the container from the sass file; I simply turned it into a styled components to get rid of the unused styles
| import { Text } from 'uiSrc/components/base/text' | ||
| import styles from './styles.module.scss' | ||
| import { Row } from 'uiSrc/components/base/layout/flex' | ||
| import * as S from './RediSearchIndexesList.styles' |
There was a problem hiding this comment.
There is single component there, why this obscurity?
There was a problem hiding this comment.
SM's style guide https://github.com/redislabsdev/cloud-ui/blob/master/style-guide/styling-themes.md#-good
THis is a scalable solution - does not changes imports in case of multiple components
There was a problem hiding this comment.
Did we adopt it officially? Since this was merged, the discussion is pointless, but I disagree with the points it makes regarding imports. When I type in the editor, it does not suggest me to import S from './styles.ts', it suggests Container.
Also in the same file there is https://github.com/redislabsdev/cloud-ui/blob/master/style-guide/styling-themes.md#-good-2 and you violate it on the spot
There was a problem hiding this comment.
re 2nd paragraph - as I said, I just moved sass styles to styled components 1:1. Both values you provided (32, 40) are different than what we had - 36. We need to come up with a solution separately regarding these values and usage across codebase.
| <S.Container> | ||
| <RiSelect.Compose | ||
| disabled={loading} | ||
| options={options} | ||
| value={index || ''} | ||
| onChange={onChangeIndex} | ||
| > | ||
| <RiSelect.Trigger.Compose data-testid="select-search-mode"> | ||
| <RiSelect.Trigger.Value | ||
| placeholder="Select Index" | ||
| data-testid="select-index-placeholder" | ||
| valueRender={selectValueRender} | ||
| /> | ||
| <RiSelect.Trigger.LoadingIndicator loading={loading} /> | ||
| <RiSelect.Trigger.Arrow data-testid="select-index-arrow" /> | ||
| <div style={{ zIndex: 6 }}> | ||
| <RiTooltip content="Refresh Indexes"> | ||
| <IconButton | ||
| size="M" | ||
| icon={ResetIcon} | ||
| disabled={loading} | ||
| onClick={handleRefresh} | ||
| aria-label="refresh indexes list" | ||
| data-testid="refresh-indexes-btn" | ||
| onPointerDown={(e) => e.stopPropagation()} | ||
| /> | ||
| </RiTooltip> | ||
| </div> | ||
| </RiSelect.Trigger.Compose> | ||
| <RiSelect.Content optionValueRender={selectValueRender} /> | ||
| </RiSelect.Compose> | ||
| </div> | ||
| </S.Container> |
There was a problem hiding this comment.
| <Container> | |
| <RiSelect.Compose | |
| disabled={loading} | |
| options={options} | |
| @@ -212,7 +214,7 @@ const RediSearchIndexesList = (props: Props) => { | |
| </RiSelect.Trigger.Compose> | |
| <RiSelect.Content optionValueRender={selectValueRender} /> | |
| </RiSelect.Compose> | |
| </Container> |
| import { Text } from 'uiSrc/components/base/text' | ||
| import styles from './styles.module.scss' | ||
| import { Row } from 'uiSrc/components/base/layout/flex' | ||
| import * as S from './RediSearchIndexesList.styles' |
There was a problem hiding this comment.
| import * as S from './RediSearchIndexesList.styles' | |
| import { Container } from './RediSearchIndexesList.styles' |
What
Testing
Note
Cursor Bugbot is generating a summary for commit ecb17de. Configure here.