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

Add hero and project information page #6030

Merged
merged 1 commit into from
Feb 6, 2025
Merged

Conversation

hom3mad3
Copy link
Contributor

@hom3mad3 hom3mad3 commented Feb 3, 2025

Description:

This PR combines two originally separate stories due to their high interdependence:

  • [#8765] Redesign frontend: Opener: Image with Aside and detail Project
  • [#8766] Redesign frontend & backend - More information page (project detail page)

Hero Project Page

Desktop View Mobile View
Desktop View Mobile View

Hero without buttons | Plan Page

Screenshot 2025-02-03 at 10-11-48 Radfahrstreifen Sonnenallee S-Bahnhof – meinBerlin

Project information page

Screenshot 2025-02-03 at 14-34-22 Stadtpark Neugestaltung 2025 -Information

Contact info section (updated in plan detail and project information)

Screenshot 2025-02-03 at 14-43-31 Stadtpark Neugestaltung 2025 -Information

Changes

register = template.Library()

@register.simple_tag
def modify_hero_content(content):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the plan detail page, we need to replace the content of the with another object (point_label) - again brings the lack of consistency in our naming throughout the apps. probably something to add to a documentation/ set of rules whenever we create a new app/model.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not lack of consistency, but different needs for the plan and project model. Plan have points for geolocation, projects do not have that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m4ra Thanks for the feedback — I get that plans and projects have different needs. What I meant by 'lack of consistency' is more about standardizing shared building blocks across models. Right now, they both have a name/title, a description, and sometimes a short description, but we're naming these differently (title vs. name, description vs. details, etc).

It’d be helpful if we kept these core properties consistent across models. That way, the code becomes more predictable and easier to work with while still giving space for each model’s unique specifics. Probably something worth documenting as a guideline for future apps.

Copy link
Contributor Author

@hom3mad3 hom3mad3 Feb 5, 2025

Choose a reason for hiding this comment

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

In this particular case, we also need to do some frontend magic because the design isn’t aware of the different building blocks. That’s another issue — and honestly, it highlights a bigger problem with how features are designed across the team.

Nowadays, we tend to build websites that are more agnostic of the content, so data models become less rigid and more reusable across the platform, that would also be an argument to unify things a little better.

@hom3mad3 hom3mad3 force-pushed the mr-2025-01-project-info-page branch 3 times, most recently from f560c13 to 8ab5969 Compare February 3, 2025 15:34
Copy link
Collaborator

@vellip vellip left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the large changes! I'd suggest waiting for @m4ra or @partizipation to also have a look regarding the django code.

meinberlin/apps/projects/views.py Outdated Show resolved Hide resolved
meinberlin/apps/projects/views.py Outdated Show resolved Hide resolved
meinberlin/assets/scss/components_user_facing/_hero.scss Outdated Show resolved Hide resolved
meinberlin/apps/plans/models.py Outdated Show resolved Hide resolved
meinberlin/apps/plans/models.py Outdated Show resolved Hide resolved
m4ra
m4ra previously requested changes Feb 5, 2025
Copy link
Contributor

@m4ra m4ra left a comment

Choose a reason for hiding this comment

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

That's a biggie indeed! Thanks for all the refactoring @hom3mad3
Few things need still some work. See my inline comments.

meinberlin/apps/plans/models.py Outdated Show resolved Hide resolved
meinberlin/apps/projects/views.py Outdated Show resolved Hide resolved
meinberlin/apps/projects/views.py Outdated Show resolved Hide resolved
register = template.Library()

@register.simple_tag
def modify_hero_content(content):
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not lack of consistency, but different needs for the plan and project model. Plan have points for geolocation, projects do not have that.

meinberlin/apps/plans/models.py Outdated Show resolved Hide resolved
@hom3mad3
Copy link
Contributor Author

hom3mad3 commented Feb 5, 2025

That's a biggie indeed! Thanks for all the refactoring @hom3mad3 Few things need still some work. See my inline comments.

@m4ra thank you for going over this long one, and for the super insightful feedback 🤗

@hom3mad3 hom3mad3 force-pushed the mr-2025-01-project-info-page branch from 8ab5969 to c9deda8 Compare February 6, 2025 11:28
@hom3mad3 hom3mad3 requested review from vellip and m4ra February 6, 2025 11:39
Copy link
Collaborator

@vellip vellip left a comment

Choose a reason for hiding this comment

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

Looks good to me. I just left one comment regarding the view. After that feel free to merge, unlees you want to wait for @m4ra ofc.

Comment on lines 401 to 403
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
return context
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can remove this if it's not needed.

I am not sure about the other two functions, they might work as is as well.

    def get_permission_object(self):
        return self.get_object()

is the default afaik

and

    @cached_property
    def project(self):
        return self.get_object()

is not used anywhere, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, it makes the code cleaner! I actually just learned that PermissionRequiredMixin defaults to get_object() for permissions, so I simplified it and kept only the basic settings.

@hom3mad3 hom3mad3 force-pushed the mr-2025-01-project-info-page branch from c9deda8 to 71672e0 Compare February 6, 2025 16:05
@hom3mad3 hom3mad3 requested a review from vellip February 6, 2025 16:09
@hom3mad3 hom3mad3 dismissed m4ra’s stale review February 6, 2025 16:11

changed based on your suggestions, but needed to merge so there are no blockers and impediments! thanks again 🙏

@hom3mad3 hom3mad3 merged commit faa8b46 into dev Feb 6, 2025
2 checks passed
@hom3mad3 hom3mad3 deleted the mr-2025-01-project-info-page branch February 6, 2025 16:25
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.

3 participants