Boton de Lectura Simple #148
Conversation
There was a problem hiding this comment.
Bravo, @danielferro69 !!! Laburazo. Ya está mergeable.
Fijate que te hice comments boludos sobre minúsculas y sobre dónde poner parte del código, por ahí vale la pena gastarle 15' más.
src/components/ChallengeView.tsx
Outdated
| @@ -75,7 +75,7 @@ const ChallengeView = (props: ChallengeViewProps) => { | |||
| const solutionParam: string = solution ? `?codigo=${solution}` : "" | |||
|
|
|||
| return <> | |||
| <Header CenterComponent={ChallengeBreadcrumb(path)} /> | |||
| <Header CenterComponent={ChallengeBreadcrumb(path)} ShouldShowSimpleReadSwitch={!path.book.simpleReadMode} /> | |||
There was a problem hiding this comment.
| <Header CenterComponent={ChallengeBreadcrumb(path)} ShouldShowSimpleReadSwitch={!path.book.simpleReadMode} /> | |
| <Header CenterComponent={ChallengeBreadcrumb(path)} shouldShowSimpleReadSwitch={!path.book.simpleReadMode} /> |
src/components/header/Header.tsx
Outdated
| @@ -32,6 +34,7 @@ export const Header = ({CenterComponent= <></>, SubHeader=<></>}: HeaderProps) = | |||
| {CenterComponent} | |||
| <div> | |||
| <ChangeLanguageButton/> | |||
| {ShouldShowSimpleReadSwitch && <SimpleReadSwitch/>} | |||
There was a problem hiding this comment.
minúscula!! Mayúscula son los componentes.
| {ShouldShowSimpleReadSwitch && <SimpleReadSwitch/>} | |
| {shouldShowSimpleReadSwitch && <SimpleReadSwitch/>} |
| const iconSx = { | ||
| backgroundColor: 'white', | ||
| borderRadius: 10, | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| color: 'gray' | ||
| } |
There was a problem hiding this comment.
Acá el ícono tiene código repetido con el DarkModeSwitch. Me pregunto si el sx no debería ponérselo el PBSwitch al ícono que recibe por parámetro. Igual es una boludez, es poquito repetido.
Y/O quizás en ambos switch (modo lectura simple y dark mode) el color y el backgroundColor podrían sacarse del tema... No sé si vale la pena. Sí me parece extraño tener estos colores hardcodeados acá. El radius y padding es propio del componente, pero ¿los colores por ahí deberían sacarse de otro lado?. Estoy pensando en voz alta.
There was a problem hiding this comment.
si, tiene codigo repetido entre otras cosas porque el PBSwitch tiene su propio color de fondo cuando fui a verlo (harcodeado a #cccccc) y suponiendo que este componente se usaba en otro lado, por eso tomé como base el darkmode y lo adapté pensando esto realmente, que el PBSwitch se usaba en otros componentes (como en el creador en el toolBoxDialog) en la aplicacion, pero ahora fui a revisar y no... asi que bien podriamos llevarlo al PBSwitch, salvo que esté pensado para usarse realmente en otro lado y por alguna cosa del destino, no ocurrió ...
src/theme/ThemeContext.tsx
Outdated
|
|
||
| const theme = useMemo( | ||
| () => createTheme(getDesignTokens(darkModeEnabled)), | ||
| [darkModeEnabled] | ||
| () => createTheme( deepmerge(getDesignTokens(darkModeEnabled), {typography: { allVariants: { textTransform: simpleReadModeEnabled ? 'uppercase': 'initial'}}})), |
There was a problem hiding this comment.
Me pregunto si esto no debería estar definido dentro de getDesignTokens al igual que el darkModeEnabled, agregándole un parámetro, para tener todo css del lado del theme
There was a problem hiding this comment.
ahi lo intento...
|
Bueno, @asanzo creo que hice todo lo sugerido. Cualquier cosa, la seguimos. |
Resolves Program-AR/pilas-bloques-ember#1252
simplereadmode.mp4