Parameters new design#1197
Merged
Merged
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:
📝 WalkthroughWalkthroughA shared ChangesParameter layout rollout
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 11
🤖 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/features/parameters/common/parameter-layout.tsx`:
- Around line 24-29: The value prop in ParameterLayoutContext.Provider is
creating a new object on every render. In the ParameterLayoutProvider function,
use the useMemo hook to memoize the value object so it only recreates when the
isXsScreen dependency changes. Store the memoized value in a variable and pass
that variable to the Provider's value prop instead of the inline object.
- Around line 8-10: Remove the unused imports `Divider` and `Button` from the
`@mui/material` import statement on line 8. Additionally, convert the icon imports
for `RestartAltIcon` and `UploadIcon` from the restricted deep-import pattern
(importing from individual icon files) to named imports directly from
`@mui/icons-material`. Apply these same corrections at the locations mentioned
(lines 97-98 and 105-106) where similar icon imports appear.
In
`@src/features/parameters/dynamic-security-analysis/dynamic-security-analysis-parameters-form.tsx`:
- Line 44: Remove the unnecessary JSX expression braces from the title prop in
the DynamicSecurityAnalysis component. The prop
`title={'DynamicSecurityAnalysis'}` violates the react/jsx-curly-brace-presence
ESLint rule because string literal values should be passed directly without
curly braces. Convert the prop syntax to use double quotes instead of curly
braces around the string value.
In
`@src/features/parameters/dynamic-simulation/dynamic-simulation-parameters-form.tsx`:
- Line 56: In the dynamic-simulation-parameters-form.tsx file, the title prop
uses unnecessary curly braces with a string literal. Remove the curly braces
around the string value for the title prop and use plain string syntax instead.
Change title={'DynamicSimulation'} to title="DynamicSimulation" to comply with
the react/jsx-curly-brace-presence ESLint rule, which requires string props to
use plain string syntax without curly braces.
In
`@src/features/parameters/network-visualizations/network-visualizations-form.tsx`:
- Line 55: In the ParameterLayout component opening tag, the title prop is using
unnecessary curly braces around a string literal. Change the title prop from
title={'NetworkVisualizations'} to title="NetworkVisualizations" by removing the
curly braces and using double quotes instead, which satisfies the
react/jsx-curly-brace-presence linting rule.
In `@src/features/parameters/pcc-min/pcc-min-parameters-form.tsx`:
- Line 53: The ParameterLayout component's title prop uses unnecessary curly
braces around the string literal, which violates the
react/jsx-curly-brace-presence linting rule. In the ParameterLayout element,
change the title prop from title={'PccMin'} to title="PccMin" to use plain
string syntax instead of JSX expression syntax.
In
`@src/features/parameters/security-analysis/security-analysis-parameters-form.tsx`:
- Line 54: In the security-analysis-parameters-form.tsx file, change the title
prop syntax from using curly braces with a string literal to plain string
syntax. Locate the line with title={'SecurityAnalysis'} and replace the curly
braces and quotes around the string value with just double quotes, so it becomes
title="SecurityAnalysis" to comply with the react/jsx-curly-brace-presence rule.
In `@src/features/parameters/sensi/sensitivity-analysis-parameters-form.tsx`:
- Line 40: The title prop in the JSX element uses unnecessary curly braces
around the string literal 'SensitivityAnalysis'. Remove the curly braces and use
direct string quotes instead, changing title={'SensitivityAnalysis'} to
title='SensitivityAnalysis' (or title="SensitivityAnalysis"). In JSX, string
literals passed as props do not require curly braces for interpolation.
In `@src/features/parameters/sensi/sensitivity-analysis-parameters-inline.tsx`:
- Around line 118-125: The `validateDisabled` property being set in the
`actions` object is not defined in the `ParameterActions` interface, so it will
be silently ignored by `ParameterLayout`. Either extend the `ParameterActions`
interface to include a `validateDisabled` boolean property, or handle the
validation logic differently by conditionally setting `validateOnClick` to
undefined within the actions object when the disabling conditions are met
(sensitivityAnalysisMethods.isLoading, isMaxResultsReached, or
isMaxVariablesReached).
In `@src/features/parameters/short-circuit/short-circuit-parameters-form.tsx`:
- Line 28: In the ParameterLayout component, remove the unnecessary curly braces
from the title prop. Change the title prop from using curly braces around the
string literal title={'ShortCircuit'} to using standard string quotes
title="ShortCircuit". This eliminates the ESLint warning about unnecessary curly
braces for string literals in JSX props.
In `@src/features/parameters/voltage-init/voltage-init-parameters-form.tsx`:
- Line 37: In the voltage-init-parameters-form.tsx file, locate the `title` prop
assignment with the value `{'VoltageInit'}` and change it to use plain string
syntax by replacing the curly braces and quotes with just double quotes around
the string value, so it becomes `title="VoltageInit"`. This aligns with the
react/jsx-curly-brace-presence ESLint rule which discourages unnecessary curly
braces around string literals in JSX props.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 070111d2-97e8-41bf-95b0-d96161e2af7a
📒 Files selected for processing (25)
src/features/parameters/common/index.tssrc/features/parameters/common/parameter-layout.tsxsrc/features/parameters/dynamic-margin-calculation/dynamic-margin-calculation-form.tsxsrc/features/parameters/dynamic-margin-calculation/dynamic-margin-calculation-inline.tsxsrc/features/parameters/dynamic-security-analysis/dynamic-security-analysis-inline.tsxsrc/features/parameters/dynamic-security-analysis/dynamic-security-analysis-parameters-form.tsxsrc/features/parameters/dynamic-simulation/dynamic-simulation-inline.tsxsrc/features/parameters/dynamic-simulation/dynamic-simulation-parameters-form.tsxsrc/features/parameters/loadflow/load-flow-parameters-form.tsxsrc/features/parameters/loadflow/load-flow-parameters-inline.tsxsrc/features/parameters/network-visualizations/network-visualizations-form.tsxsrc/features/parameters/network-visualizations/network-visualizations-parameters-inline.tsxsrc/features/parameters/parameters-style.tssrc/features/parameters/pcc-min/pcc-min-parameters-form.tsxsrc/features/parameters/pcc-min/pcc-min-parameters-inline.tsxsrc/features/parameters/security-analysis/security-analysis-parameters-form.tsxsrc/features/parameters/security-analysis/security-analysis-parameters-inline.tsxsrc/features/parameters/sensi/sensitivity-analysis-parameters-form.tsxsrc/features/parameters/sensi/sensitivity-analysis-parameters-inline.tsxsrc/features/parameters/short-circuit/short-circuit-parameters-form.tsxsrc/features/parameters/short-circuit/short-circuit-parameters-inline.tsxsrc/features/parameters/voltage-init/voltage-init-parameters-form.tsxsrc/features/parameters/voltage-init/voltage-init-parameters-inline.tsxsrc/translations/en/commonButtonEn.tssrc/translations/fr/commonButtonFr.ts
💤 Files with no reviewable changes (1)
- src/features/parameters/parameters-style.ts
khouadrired
force-pushed
the
parameters-new-design
branch
2 times, most recently
from
850236b to
78e38bd
Compare
khouadrired
force-pushed
the
parameters-new-design
branch
from
78e38bd to
a4bb449
Compare
FranckLecuyer
requested changes
Jun 22, 2026
FranckLecuyer
requested changes
Jun 22, 2026
khouadrired
force-pushed
the
parameters-new-design
branch
from
db9f8a5 to
3f7527a
Compare
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/translations/en/parameters.ts`:
- Line 478: Fix the spelling errors in the tooltip.reset translation string.
Locate the tooltip.reset key in the parameters translation file and correct the
two typos in its value: change "defaut" to "default" and change "paramaters" to
"parameters" so the tooltip text reads "User default parameters".
In `@src/translations/fr/parameters.ts`:
- Line 501: In the src/translations/fr/parameters.ts file, locate the
'tooltip.reset' key on line 501 and change the French translation from the
singular "Paramètre par défaut du profil utilisateur" to the plural form
"Paramètres par défaut du profil utilisateur" to maintain consistency with the
English plural semantics and match the pattern used elsewhere in the file at
lines 20, 21, and 125.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e34e4c57-ace6-4b61-903c-49bb178b41f3
📒 Files selected for processing (15)
src/features/parameters/common/parameter-layout.tsxsrc/features/parameters/dynamic-margin-calculation/dynamic-margin-calculation-form.tsxsrc/features/parameters/dynamic-security-analysis/dynamic-security-analysis-parameters-form.tsxsrc/features/parameters/dynamic-simulation/dynamic-simulation-parameters-form.tsxsrc/features/parameters/loadflow/load-flow-parameters-form.tsxsrc/features/parameters/loadflow/load-flow-parameters-inline.tsxsrc/features/parameters/network-visualizations/network-visualizations-form.tsxsrc/features/parameters/pcc-min/pcc-min-parameters-form.tsxsrc/features/parameters/security-analysis/security-analysis-parameters-form.tsxsrc/features/parameters/sensi/sensitivity-analysis-parameters-form.tsxsrc/features/parameters/sensi/sensitivity-analysis-parameters-inline.tsxsrc/features/parameters/short-circuit/short-circuit-parameters-form.tsxsrc/features/parameters/voltage-init/voltage-init-parameters-form.tsxsrc/translations/en/parameters.tssrc/translations/fr/parameters.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/features/parameters/network-visualizations/network-visualizations-form.tsx
- src/features/parameters/dynamic-security-analysis/dynamic-security-analysis-parameters-form.tsx
- src/features/parameters/sensi/sensitivity-analysis-parameters-inline.tsx
- src/features/parameters/loadflow/load-flow-parameters-inline.tsx
- src/features/parameters/dynamic-margin-calculation/dynamic-margin-calculation-form.tsx
- src/features/parameters/common/parameter-layout.tsx
- src/features/parameters/voltage-init/voltage-init-parameters-form.tsx
- src/features/parameters/security-analysis/security-analysis-parameters-form.tsx
- src/features/parameters/short-circuit/short-circuit-parameters-form.tsx
- src/features/parameters/sensi/sensitivity-analysis-parameters-form.tsx
- src/features/parameters/loadflow/load-flow-parameters-form.tsx
- src/features/parameters/pcc-min/pcc-min-parameters-form.tsx
- src/features/parameters/dynamic-simulation/dynamic-simulation-parameters-form.tsx
khouadrired
force-pushed
the
parameters-new-design
branch
from
3f7527a to
8bc327f
Compare
FranckLecuyer
approved these changes
Jun 22, 2026
TheMaskedTurtle
requested changes
Jun 22, 2026
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
Contributor
There was a problem hiding this comment.
- I can still see a lot of custom sx here and there. It would be a great occasion to rationalize that and to limit them to what is really useful. It leads to a lot of little differences between screens, so the less we have them the better we stand. So, try to limit them as much as possible.
- I think you should use Grid2 directly with import { Grid2 as Grid } to avoid second time refactoring, and rather use Grid than Stack or Box, for project consistency.
- I am not a big fan of putting a lot of things in the header section, for me it should be limited to the title. If we want something specific for the provider I would rather add a specific section provider in the parameter layout, instead of using the header one. Otherwise I would just let it be handled by the content itself.
- I am also surprised by the renderTitleFields present in the forms, not sure it should be here, but maybe that would be something to dig deeper int the future.
Could you add more description in the PR about which functionality it covers and add some screenshots of the changes.
Thanks 🙏
khouadrired
force-pushed
the
parameters-new-design
branch
from
96d3e21 to
2335dae
Compare
TheMaskedTurtle
requested changes
Jun 23, 2026
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
Contributor
There was a problem hiding this comment.
According to me, there is a misconception about components hierarchy here. The *Form components should contain the whole form, provider and tabs included, and it should be agnostic of the surrounding environment. Then, in gridstudy-app the Tab content should be your newly introduced Layout with the Form as content. In gridexplore-app, it should be a MuiDialog with inside stacked NameElementEditorForm, followed by the parameter Form. Here it is the form that contains the layout, and the inline that contains the form, it causes many optional fields in your layout to match both behaviors when in reality it is not used in gridexplore-app. We can talk further about this, but I think it is really important to reverse the logic.
Contributor
|
Is there a plan to use the ParameterLayoutContext in the future ? For now, I feel like it is not used and it might be a little over-engineered, maybe we can introduce it only when it will be used. |
thangqp
requested review from
antoinebhs,
ayolab,
flomillot and
klesaulnier
as code owners
TheMaskedTurtle
requested changes
Jun 25, 2026
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
Contributor
There was a problem hiding this comment.
- It needs to be tested extensively, to be sure all layouts in both gridexplore and gridstudy are clean, I haven't done it but just on loadflow parameters I noticed regressions.
- Still not convinced by the "small screen handling".
- Except for the small points I raised (about
extraprop and NameElementEditorForm), I think in terms of component hierarchy and responsabilities, it is perfect like that, thanks for the rework :)
Contributor
Screenshot to prove my point about small screens, it's not like it's really useable anyway ^^ |
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
thangqp
approved these changes
Jul 2, 2026
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
khouadrired
force-pushed
the
parameters-new-design
branch
from
9ebd410 to
dddc028
Compare
TheMaskedTurtle
approved these changes
Jul 3, 2026
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
Contributor
There was a problem hiding this comment.
Ok for the architecture. Thanks for the refactoring !
I still think someone from your team should test all of this very carefully again, especially on the error management in tabs, I think there is a risk. I haven't performed comprehensive functionnal tests, I just approve the core concept of the refactoring.
Contributor
This PR is not merge before the migration Prs Grid -> Grid2. And then, they layout component will be migrated to Grid2 before retesting then merge.. |
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR standardizes the layout and action handling across all 10 parameter categories in the application. It migrates all parameter forms to a unified
ParameterLayoutcomponent and replaces the legacyrenderActionspattern with a centralizedParameterActionsinterface.Key Changes
ParameterLayoutfor consistent scrolling behavior and responsive design.ParameterActionsobject to group standard callbacks (Save, Reset, Validate, Choose Settings) and anextraprop for custom dialogs/modals.ParametersTabsto ensure content remains visible when using the top-level selection component.maxHeightand absolute positioning)Benefits
All parameter forms now share the same structure: Header -> Scrollable Content -> Sticky Footer.
Reduced boilerplate by passing a single
actionsobject instead of multiple individual props.