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

redesign phase info component #5385

Merged
merged 7 commits into from
Jan 23, 2024
Merged

redesign phase info component #5385

merged 7 commits into from
Jan 23, 2024

Conversation

hom3mad3
Copy link
Contributor

@hom3mad3 hom3mad3 commented Jan 3, 2024

Redesign Phase Info Component

Test Page

projekte/module/module-title (e.g., Participatory Budgeting Phase 3)

Changes

  • Implemented redesign for the Phase Info Panel component. Uses Servicepanel and custom style overwrites for the swiper.
  • Enhanced for both desktop and mobile views with Swiper integration.

Previews

  • Desktop: PhaseInfo-Desktop
  • Mobile:

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Changelog

@hom3mad3 hom3mad3 changed the title Mr 2024 01 phase info component WIP Mr 2024 01 phase info component Jan 3, 2024
@hom3mad3 hom3mad3 changed the title WIP Mr 2024 01 phase info component [7762] re-design phase info component Jan 3, 2024
@hom3mad3 hom3mad3 changed the title [7762] re-design phase info component wip [7762] re-design phase info component Jan 3, 2024
@hom3mad3 hom3mad3 changed the title wip [7762] re-design phase info component re-design phase info component Jan 9, 2024
@hom3mad3 hom3mad3 changed the title re-design phase info component redesign phase info component Jan 9, 2024
@hom3mad3 hom3mad3 force-pushed the mr-2024-01-phase-info-component branch from b3f37b6 to a01b612 Compare January 9, 2024 14:03
@hom3mad3
Copy link
Contributor Author

hom3mad3 commented Jan 22, 2024

@goapunk @philli-m @hom3mad3 i have absolutely no idea how and where to test this, as EverythingWorksOnMyMachine™ but tests are failing :D

@@ -1,61 +1,41 @@
// import { Swiper, Pagination, A11y } from 'swiper'
// import django from 'django'
import Swiper from 'swiper/bundle'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

works for me™ and follows swiper documentation. not sure what else to do

@philli-m
Copy link
Contributor

@goapunk @philli-m @hom3mad3 i have absolutely no idea how and where to test this, as EverythingWorksOnMyMachine™ but tests are failing :D

@hom3mad3 I'm not sure what this comment is concerning? should we have a look at something specifically?

@hom3mad3
Copy link
Contributor Author

@goapunk @philli-m @hom3mad3 i have absolutely no idea how and where to test this, as EverythingWorksOnMyMachine™ but tests are failing :D

@hom3mad3 I'm not sure what this comment is concerning? should we have a look at something specifically?

failed tests:

Error: 1:20 error Unable to resolve path to module 'swiper/bundle' import/no-unresolved
Error: 2:8 error Unable to resolve path to module 'swiper/css/bundle' import/no-unresolved

The tests are failing in our testing environment, and I don't have the means to debug the issue as I'm unable to replicate the problem locally.

@philli-m philli-m self-assigned this Jan 22, 2024
@philli-m
Copy link
Contributor

@hom3mad3 hmm that's probably because we no longer install swiper as the BO styleguide to avoid a doubling up, but it is weird it's not failing locally, I'll have a look, maybe we need to import it differently or something

@philli-m
Copy link
Contributor

@hom3mad3 checking this i'm still not sure why the additional swiper.js is required? the swiper should be initialized by the BO js, is it just the style overwrites? maybe we can talk tomorrow?

@hom3mad3
Copy link
Contributor Author

hom3mad3 commented Jan 22, 2024

@hom3mad3 checking this i'm still not sure why the additional swiper.js is required? the swiper should be initialized by the BO js, is it just the style overwrites? maybe we can talk tomorrow?

this was the only way i could find to be able to style the swiper pagination and make it accessible, otherwise i cannot access the Swiper object which as been instantiated by the BO component and add customizations. i removed the modul-buhne element which initializes the swiper and was causing the issue.

@philli-m philli-m force-pushed the mr-2024-01-phase-info-component branch from e9a7d06 to f19c276 Compare January 23, 2024 11:17
@@ -50,7 +50,7 @@
@import "../berlin_css/berlin_marketing";

// extra scss / overwrites
@import "../extra_css/swiper.scss";
@import "../extra_css/swiper";
Copy link
Contributor

Choose a reason for hiding this comment

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

removed as didn't pass linter, not sure why there wasn't an error when commiting, maybe we need to check out commit linting?

@@ -26,7 +34,7 @@ const initSwiper = () => {
}

createSwiper(config)
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

also lint error

import 'swiper/css'
import 'swiper/css/navigation'
import 'swiper/css/pagination'
/* eslint-enable import/no-unresolved */
Copy link
Contributor

@philli-m philli-m Jan 23, 2024

Choose a reason for hiding this comment

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

This is apparently a known issue, have added link, also just did a little update to only import the used modules as the bundle is a bit big also we don't seem to use any of the A11y module options so didn't add it, but not sure if i'm reading it all right

@philli-m philli-m force-pushed the mr-2024-01-phase-info-component branch from f19c276 to 67c284c Compare January 23, 2024 11:22
@philli-m
Copy link
Contributor

@hom3mad3 checking this i'm still not sure why the additional swiper.js is required? the swiper should be initialized by the BO js, is it just the style overwrites? maybe we can talk tomorrow?

this was the only way i could find to be able to style the swiper pagination and make it accessible, otherwise i cannot access the Swiper object which as been instantiated by the BO component and add customizations. i removed the modul-buhne element which initializes the swiper and was causing the issue.

fair play, might make the liberary integration a bit of a pain but I am hoping that will not be our job :p
It's looking good! I fixed the linting so the tests should pass now, just to check do you get errors when you commit? the staged linting should have picked up on that but it can break sometimes :(

Happy to merge when tests pass, just left some comments as change the import a bit, let me know if that's ok?!

@philli-m philli-m removed their assignment Jan 23, 2024
@hom3mad3
Copy link
Contributor Author

hom3mad3 commented Jan 23, 2024

@philli-m thanks for taking the time to figure this out. as expected, the code doesn't work for me locally anymore, probably due to the import changes :( this really is a problem, that we get to work with such different local environments and they don't necessarily reflect the server settings.
good news is that linting works: .../assets/js/swiper_phases.js 1:20 error Unable to resolve path to module 'swiper' import/no-unresolved

all in all i am extremely happy and relieved to get this merged

@m4ra @goapunk just as a heads up, as this is likely to keep happening

@philli-m philli-m merged commit 5af10ed into dev Jan 23, 2024
2 checks passed
@philli-m philli-m deleted the mr-2024-01-phase-info-component branch January 23, 2024 14:23
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.

2 participants