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

front: Fix cartography sub windows #2876

Merged
merged 15 commits into from
Feb 20, 2023
Merged

Conversation

Uriel-Sautron
Copy link
Contributor

close #2831

@Uriel-Sautron Uriel-Sautron requested a review from a team as a code owner January 13, 2023 09:11
@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #2876 (3f8ab5f) into dev (f3fd9df) will increase coverage by 0.19%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##                dev    #2876      +/-   ##
============================================
+ Coverage     40.48%   40.68%   +0.19%     
  Complexity     1711     1711              
============================================
  Files           630      634       +4     
  Lines         18805    18724      -81     
  Branches       2360     2346      -14     
============================================
+ Hits           7614     7618       +4     
+ Misses        10777    10692      -85     
  Partials        414      414              
Flag Coverage Δ
front 4.85% <0.00%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
...rainSchedule/ManageTrainScheduleMap/RenderPopup.js 0.00% <ø> (ø)
front/src/common/Map/Buttons/MapButtons.js 0.00% <0.00%> (ø)
front/src/common/Map/HeaderPopUp.tsx 0.00% <0.00%> (ø)
front/src/common/Map/MapKey.tsx 0.00% <0.00%> (ø)
front/src/common/Map/Search/MapSearch.js 0.00% <0.00%> (ø)
front/src/common/Map/Settings/MapSettings.js 0.00% <0.00%> (ø)
front/src/common/Map/const.ts 100.00% <ø> (ø)
front/src/utils/hooks/useOutsideClick.ts 0.00% <0.00%> (ø)
front/src/applications/operationalStudies/Home.js 0.00% <0.00%> (ø)
...t/src/applications/stdcm/views/OSRDStdcmResults.js 0.00% <0.00%> (ø)
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@alexandredamiron
Copy link
Contributor

Even if there is no conflict, could you rebase? It's hard to test. Thank you

@Uriel-Sautron
Copy link
Contributor Author

Rebase done but work to replace eventListener in MapButton.js

@Uriel-Sautron
Copy link
Contributor Author

Create custom hook useOutsideClick to use in MapButtons.

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.

Nice job ! I have left several comments.

I think the modals (or popovers) do not need to have active in their props, since they should not be displayed if they are not active. Some refacto can be done if you remove active of each component (MapSearch, MapSettings, MapKey)

front/src/common/Map/MapKey.js Outdated Show resolved Hide resolved
front/src/common/Map/Search/MapSearch.js Outdated Show resolved Hide resolved
front/src/common/Map/Buttons/MapButtons.js Outdated Show resolved Hide resolved
front/src/common/Map/Search/MapSearch.js Outdated Show resolved Hide resolved
front/src/common/Map/Search/MapSearch.js Outdated Show resolved Hide resolved
front/src/common/Map/Settings/MapSettings.js 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.

Some small comments to fix

front/src/common/Map/Buttons/MapButtons.js Outdated Show resolved Hide resolved
front/src/common/Map/Buttons/MapButtons.js Outdated Show resolved Hide resolved
front/src/common/Map/Buttons/MapButtons.js Outdated Show resolved Hide resolved
front/src/common/Map/Settings/MapSettings.js Outdated Show resolved Hide resolved
front/src/common/Map/Search/MapSearch.js Outdated Show resolved Hide resolved
front/src/common/Map/Settings/MapSettings.js Outdated Show resolved Hide resolved
front/src/common/Map/MapKey.tsx Outdated Show resolved Hide resolved
front/src/common/Map/Map.scss Show resolved Hide resolved
…gs: change function name. Map.scss: remove active in map-modal class
clarani
clarani previously approved these changes Feb 20, 2023
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.

Nice job

… modal and an useEffect to close the map popup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cartography sub-windows do not respond as we'd want
4 participants