ui-manchette : integration get manchette #86
Conversation
Math-R
force-pushed
the
mrd/ui-manchette
branch
2 times, most recently
from
28fa47c to
0916d3d
Compare
Math-R
force-pushed
the
mrd/ui-manchette
branch
2 times, most recently
from
9acd20c to
6d3d07a
Compare
Math-R
force-pushed
the
mrd/ui-manchette
branch
6 times, most recently
from
52ce002 to
f355fb8
Compare
| @@ -36,7 +36,8 @@ | |||
| "dependencies": { | |||
| "classnames": "^2.5.1", | |||
| "lodash.isequal": "^4.5.0", | |||
| "tailwindcss": "^3.4.1" | |||
| "tailwindcss": "^3.4.1", | |||
There was a problem hiding this comment.
Since this @osrd-project/ui-manchette module exports its own built CSS code, why is TailWind in the dependencies? Can I use it without Tailwind? And if I use Tailwind in my project that depends on @osrd-project/ui-manchette, what is the benefit?
|
|
||
| const zoomIn = () => { | ||
| if (zoom < 10) setZoom(zoom + 0.5); | ||
| const zoomYIn = () => { |
There was a problem hiding this comment.
It would be nice to have those three constants (min, max, step) stored and named somewhere, like in a consts.ts file somewhere. I would even argue to make them overridable through props.
|
|
||
| export const calcOperationalPointsToDisplay = ( | ||
| operationalPoints: OperationalPointType[], | ||
| isProportionnal: boolean, |
There was a problem hiding this comment.
I only point it here, but there are a lot of typos with proportionnal that should be proportional with one single N in the manchette codebase.
| } | ||
| }; | ||
|
|
||
| export const operationalPointsHeight = ( |
There was a problem hiding this comment.
I see that in this function, you mutate existing styles attributes for the input operationalPoints.
I think this pattern is dangerous: When I see this function that takes a StyledOperationalPointType[] and returns another StyledOperationalPointType[], I would expect it not to mutate what's inside the input value:
- Either you return something totally new
- Either you mutate, and name and type the function accordingly (like
const setOperationalPointsHeight: (...) => voidfor instance)
I know JavaScript itself does not respect this convention (like with Array#reverse for instance), but it has been widely criticized, and it is now quite standard to have no side effects on functions that return something new.
Anyway, a common way to avoid mutating the input would be to replace the op.styles = {...}; return op; by return {...op, styles: {...}};.
| @@ -1,3 +1,5 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Only import { CSSProperties } instead would be better I think
| positions: number[]; | ||
| times: number[]; | ||
| }[]; | ||
| } & { |
There was a problem hiding this comment.
Why are there two types that are combined with { ... } & { ... } instead of one single type?
| const Manchette: React.FC<ManchetteProps> = ({ operationalPoints, projectPathTrainResult }) => { | ||
| const manchette = useRef<HTMLDivElement>(null); | ||
|
|
||
| const [zoomY, setZoomY] = useState(1); |
There was a problem hiding this comment.
Why do you use so many separate setState instead of one single "larger" setState such as something like this for instance?
const [state, setState] = useState({
zoomX: 1,
zoomY: 1,
offsetX: 0,
offsetY: 0,
// ...
});The "more compact" version seems easier to read for me (it's easy to have a full overview of what's in the state). Do you see any pros on your "split" version?
This commit fixes various comments from my review for PR #86. The PR has been merged while I was reviewing it, so we decided I can directly submit a new one with my fixes. Details: - Fixes all typos from proportionnal to proportional - Fixes React.XXX as import { XXX } from 'react' - Refactors Manchette.tsx code to have a single, more readable setState - Removes magic zoom boundaries and replace them with proper consts - Fixes operationalPointsHeight so that it does not mutate things anymore - Rewrites calcOperationalPointsToDisplay with simpler code and some comment, to help make it maintainable - Actually stops displaying OperationalPoint with display: false
This commit fixes various comments from my review for PR #86. The PR has been merged while I was reviewing it, so we decided I can directly submit a new one with my fixes. Details: - Fixes all typos from proportionnal to proportional - Fixes React.XXX as import { XXX } from 'react' - Refactors Manchette.tsx code to have a single, more readable setState - Removes magic zoom boundaries and replace them with proper consts - Fixes operationalPointsHeight so that it does not mutate things anymore - Rewrites calcOperationalPointsToDisplay with simpler code and some comment, to help make it maintainable - Actually stops displaying OperationalPoint with display: false
No description provided.