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.

Sign up for GitHub

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

Jump to bottom

Add Lyon Cluster Map #640

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Lyon Cluster Map #640

wants to merge 5 commits into from

Conversation

gd-harco
Copy link

@gd-harco gd-harco commented Oct 10, 2024 •
edited by 42atomys
Loading

Describe the pull request
Added the Lyon cluster map in web/ui/src/lib/ClustersMaps.
Not sure about the REGEX part, I'm pretty bad with these.

Checklist

  • I have made the modifications or added tests related to my PR
  • I have run the tests and linters locally and they pass
  • I have added/updated the documentation for my RP

Additional context

Couldn't test it du to the issue with dev containers, but should work as intended.

@gd-harco gd-harco requested a review from 42atomys as a code owner October 10, 2024 11:13
@gd-harco gd-harco mentioned this pull request Oct 10, 2024
Open
1 task
42atomys
42atomys reviewed Oct 10, 2024
View reviewed changes
Copy link
Owner

@42atomys 42atomys left a comment

Choose a reason for hiding this comment

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

Hi, thanks for open your Pull request, I do an explicative review for the regexp and allow the run of CI to ensure all are good !

web/ui/src/lib/clustersMap/campus/lyon.ts Outdated Show resolved Hide resolved
42atomys
42atomys approved these changes Oct 10, 2024
View reviewed changes
Copy link
Owner

@42atomys 42atomys left a comment

Choose a reason for hiding this comment

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

Linters and Test are green ✅

Congrats to do it without linters 🎉

@42atomys 42atomys linked an issue Oct 10, 2024 that may be closed by this pull request
feat: integrate lyon campus #319
Open
1 task
@gd-harco
Copy link
Author

Thanks for the explanation !
I manage to use the linter in the dev container, so I don't have much merit 😅
Please feel free to @ me if some things need changes.

We will be receiving new computers in the month, so I'll be sure to create a new PR to update the map !

42atomys
42atomys approved these changes Oct 12, 2024
View reviewed changes
Copy link
Owner

@42atomys 42atomys left a comment

Choose a reason for hiding this comment

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

All seams to be good LGTM 🎉✅

I will send a mail to Lyon campus staff to allow usage of the building map and the PR will be merged at this time 💜

@42atomys 42atomys added type/improvement ✨ Improvement to an existing feature good first issue Good for newcomers state/needs information 🚧 We needs more information to go forwards priority/medium 🟨 Priority 3 - Not blocking but should be fixed soon aspect/interface 🕹 Concerns end-users' experience with the software domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously labels Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@42atomys 42atomys 42atomys approved these changes

Assignees
No one assigned
Labels
aspect/interface 🕹 Concerns end-users' experience with the software domain/obvious 🟩 Represents the "known knowns" issue. It's Obviously good first issue Good for newcomers priority/medium 🟨 Priority 3 - Not blocking but should be fixed soon size/L state/needs information 🚧 We needs more information to go forwards type/improvement ✨ Improvement to an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: integrate lyon campus
2 participants
@gd-harco @42atomys