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

More feedbacks incl. major refactoring #612

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

More feedbacks incl. major refactoring #612

wants to merge 6 commits into from

Conversation

fivaldi
Copy link
Contributor

@fivaldi fivaldi commented Jun 4, 2020 •
edited
Loading

  • most of the moving data in one place (cities/ directory)
  • support for per city feedbacks (incl. score 0..5)
  • 'about' data moved away from templates
  • partners as YAML
  • thoroughly tested incl. partners' links and redirects
  • PEP8 cleanup
  • do pytest and flake8 within Travis CI

See #611 for original motivation.

@fivaldi
Major code refactoring:
9b8bcf2 
- most of the data in one place (cities/ directory)
- support for per city feedbacks (incl. score 0..5)
- 'about' data moved away from templates
- partners as YAML
- thoroughly tested incl. partners' links and redirects
@fivaldi fivaldi force-pushed the fivaldi_more_feedbacks branch 2 times, most recently from 4976e8a to 25e9ad5 Compare June 4, 2020 09:13
@fivaldi
CI fixes:
180b48f 
- PEP8 cleanup
- do pytest and flake8 within Travis CI
@fivaldi
Copy link
Contributor Author

fivaldi commented Jun 4, 2020 •
edited
Loading

@frenzymadness A little more (nightly) code than expected :-) but should be ready for review.

@encukou
Copy link
Member

encukou commented Jun 4, 2020

Je nějaká PyLady, která tomu refactoringu rozumí?
(Já bych review udělal, ale myslím si že já nebo Lumír nejsem ti správní...)

@fivaldi
Copy link
Contributor Author

fivaldi commented Jun 4, 2020 •
edited
Loading

obrazek

Plus ještě jedna (bonusová) featura v 7df3677

@frenzymadness
Copy link
Contributor

Není to trochu moc změn najednou? Těžko se o nich bude diskutovat, když jich je taková hromada.

@fivaldi
Copy link
Contributor Author

fivaldi commented Jun 5, 2020 •
edited
Loading

Není to trochu moc změn najednou? Těžko se o nich bude diskutovat, když jich je taková hromada.

Je. 🙊

@fivaldi
Copy link
Contributor Author

fivaldi commented Jun 5, 2020

No. Kvůli deploymentu do statických stránek je ta feedbacková featura stejně asi "no go", takže jak dál... Nějaký nápad? Ten JS?

@fivaldi
Rotate feedbacks using JS:
a706dd9 
- Reasonable / robust enough solution using jQuery.
- Ready for static (''frozen'') deployment.
- Add first Olomouc feedbacks, thanks @verka and @hanka.
- HTML still works independently. No need of JS for initial feedbacks show up.
@fivaldi
Copy link
Contributor Author

fivaldi commented Jun 6, 2020 •
edited
Loading

Přidal jsem ten JS. Ono to asi jinak dost dobře nepůjde.

Je toho tady hodně, tak když tak bych to rozbil na menší části a více PR:

  1. Refactoring struktury a přemístění YAML. Pro nerozbití by v tom byla ta většina testů.
  2. Přidání feedbackové featury: statická + JS
  3. Materiály: Obsahově dynamické dropdown menu s možností statických položek
  4. Partneři.

Nebo to prostě odrevidovat a zkusit nasadit. Když to neklapne, tak se vrátit a pak řešit, co jak dál. Vizuálně / obsahově-vizuálně jsem to testovat lokálně, responzivitu to neafektuje...

Jako follow up by bylo dobré ujednotit názvy kurzů (menu s materiály). Linie názvů v Hradci, Plzni, Olomouci (:-)) a Brně je OK. :-)

@fivaldi
Copy link
Contributor Author

fivaldi commented Sep 3, 2020

Do you want me to bring this back to life (splitting stuff ), or rather not? :-) Thanks for opinions/thumbs-ups/-downs to know if it really matters or not.

Also, please see the initial comment (and the others) for the descriptions of features/changes/new tests...

@encukou
Copy link
Member

encukou commented Sep 9, 2020

I don't currently see interest in reviewing this.
IMO this needs a PyLady who understands both the current state and the refactoring, and ideally would be willing to maintain the site.

@fivaldi
Copy link
Contributor Author

fivaldi commented Sep 9, 2020

@encukou Thank you. I've got one candidate, our PyLady @FiVerka , so will ask her tomorrow if she's willing to take this. If yes, I'd explain her the details ...

@fivaldi
Copy link
Contributor Author

fivaldi commented Sep 11, 2021 •
edited
Loading

Please for discontinuation of this PR, unless someone else raises the benefits of the fixes/enhancements here, e.g. new feedbacks/tests. This would be a motivation for me to rework it from scratch. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fivaldi @encukou @frenzymadness