Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cnh/intregate UI core from npm #7162

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Conversation

kmer2016
Copy link
Contributor

@kmer2016 kmer2016 commented Apr 10, 2024

Testing the integration of ui-core into osrd/front.

⚠️ Do not merge before removing the last commit, which is only there to test the integration of the ui-core input into an osrd form (rollingstockEditor).
image

To handle the integration properly, we need to reset some styles. This is due to the base style of Tailwind that comes with ui-core, which sets the default style for these HTML elements. In particular :

  • the display of img and svg is set to display:block, which positions them above or below their sibling elements. (issue reported here).

  • the height of the img is set to auto, which causes the img to take up all the available space of the parent element even if you define a height in the style (issue reported here).

  • The button has a background-color transparent as mentionned here

@kmer2016 kmer2016 force-pushed the cnh/intregate_ui_core_from_npm branch from cbf2389 to bce3397 Compare April 10, 2024 09:05
@kmer2016 kmer2016 mentioned this pull request Apr 10, 2024
3 tasks
@kmer2016 kmer2016 added the area:front Work on Standard OSRD Interface modules label Apr 10, 2024
@kmer2016 kmer2016 linked an issue Apr 10, 2024 that may be closed by this pull request
3 tasks
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I try to focus an input with tab, I get this :

image

Is that normal ?

Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, I spotted some bugs:

  • display is broken if I click on the arrow next to the eye in the scenario page 👁️
    image

  • the display of the simulation parameters is not correct, same for the import tab and the stdcm page
    image
    image
    image

  • the rolling stock image is very small in the rolling stock selector
    image

  • the label is transparent on the editor
    image

  • the selector modal is broken in the editor
    image

  • the popover to search the map is too large
    image

  • the popover to select the data layer is also broken in the main map
    image

front/src/styles/styles.scss Outdated Show resolved Hide resolved
@kmer2016 kmer2016 force-pushed the cnh/intregate_ui_core_from_npm branch from bce3397 to e2dd555 Compare April 24, 2024 13:43
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 28.73%. Comparing base (95493fe) to head (88e8376).

Files Patch % Lines
...plications/operationalStudies/views/ScenarioV1.tsx 0.00% 2 Missing ⚠️
...cations/operationalStudies/views/v2/ScenarioV2.tsx 0.00% 2 Missing ⚠️
...src/applications/editor/components/LayersModal.tsx 0.00% 1 Missing ⚠️
...mon/Map/Settings/MapSettingsBackgroundSwitches.tsx 0.00% 1 Missing ⚠️
...ront/src/common/Map/Settings/MapSettingsLayers.tsx 0.00% 1 Missing ⚠️
front/src/common/Pathfinding/Pathfinding.tsx 0.00% 1 Missing ⚠️
...t/src/modules/scenario/components/ScenarioCard.tsx 0.00% 1 Missing ⚠️
front/src/modules/study/components/StudyCard.tsx 0.00% 1 Missing ⚠️
...hedule/components/Timetable/TimetableTrainCard.tsx 0.00% 1 Missing ⚠️
...le/components/TimetableV2/TimetableTrainCardV2.tsx 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #7162      +/-   ##
============================================
- Coverage     28.73%   28.73%   -0.01%     
  Complexity     2004     2004              
============================================
  Files          1123     1123              
  Lines        138057   138057              
  Branches       2684     2684              
============================================
- Hits          39670    39668       -2     
- Misses        96790    96792       +2     
  Partials       1597     1597              
Flag Coverage Δ
core 78.37% <ø> (ø)
editoast 72.10% <ø> (-0.01%) ⬇️
front 9.27% <0.00%> (ø)
gateway 2.43% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 83.96% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kmer2016 kmer2016 force-pushed the cnh/intregate_ui_core_from_npm branch from e2dd555 to f715157 Compare April 24, 2024 13:44
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface is perfect now, thanks, I only noticed that the input does not seem to work and you forgot a new line 👍

front/src/styles/scss/_uiCoreIntegration.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅ You can merge after you have done these few things:

  • fix the missing new line
  • remove the commit which test the new input component

The bugs related to the input will be fixed later in osrd-ui

@kmer2016 kmer2016 force-pushed the cnh/intregate_ui_core_from_npm branch 2 times, most recently from 37e398b to 5a14c17 Compare April 25, 2024 11:12
@kmer2016 kmer2016 marked this pull request as ready for review April 25, 2024 11:19
@kmer2016 kmer2016 requested a review from a team as a code owner April 25, 2024 11:19
front: fix forgotten img

front: fix notification buttons on map editor
@kmer2016 kmer2016 force-pushed the cnh/intregate_ui_core_from_npm branch from 5a14c17 to 88e8376 Compare April 25, 2024 11:22
@kmer2016 kmer2016 added this pull request to the merge queue Apr 25, 2024
Merged via the queue into dev with commit 11da135 Apr 25, 2024
17 checks passed
@kmer2016 kmer2016 deleted the cnh/intregate_ui_core_from_npm branch April 25, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare UI-core integration
4 participants