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

Hide Materialfilter PeriodOnly in ContentNode #4361

Conversation

MarcoAllenspach
Copy link
Contributor

fix #3862
I fixed the issu plus i added also the filter possibility for activity only i hope thats ok ;)
This is my first pull request please check the change
And please give me feedback :)

image
image
image

Marco and others added 4 commits December 28, 2023 17:21
Show filter just if activity and a period material item is in the list
Add filter for activity material
@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Dec 31, 2023
Copy link

github-actions bot commented Dec 31, 2023

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

@manuelmeister
Copy link
Member

manuelmeister commented Dec 31, 2023

Hey Marco,

Thanks for diving into this with your first PR – it’s great to see your enthusiasm. Regarding issue #3862, our main goal is to hide the Materialfilter PeriodOnly in the activity view. This is because the Activity / Period filter isn’t relevant here, as the section doesn’t display period material. I've included some screenshots to show what I mean:

Bildschirmfoto 2023-12-31 um 17 05 53
Bildschirmfoto 2023-12-31 um 17 06 14

I noticed your PR doesn’t quite work as you might expect. To align with the goal of #3862, you might want to provide an additional prop to disable the filter in the content node (also computationally expensive calculations such as showFilter should be a computed prop). If you need more detailed guidance or have questions, feel free to ask!

Also, I see you added an activity-only filter. It’s an interesting idea, but it’s a bit outside the scope of this issue. How about we track this as a separate feature? You could open a new issue for it, and we can discuss it further there.

Looking forward to your updates, and I'm here to help if you need it!

@MarcoAllenspach
Copy link
Contributor Author

Salutti Manuel

I created a new issue
#4364

Pleas have a lock over the new solution:
devel...MarcoAllenspach:ecamp3:fixedHideMaterialFilter
What do you think?

I can also make a prop which i can set in the contend node
But with the other solution it also not show the filter in Materials if no period material is in the list, what makes sense to me

@manuelmeister
Copy link
Member

Nice work!

I checked out your proposed solution and it coming along nicely.

For the activity view, I still believe removing the filter button completely would be best. It simplifies the UI and avoids confusion. So, using a prop in the content node that bypasses the showFilter computation and overrides the visibility of the button, would make sense.

Regarding the material view, I’m still thinking about whether the button should even be disabled or removed. Your approach of hiding the filter when there's no period material is interesting.

Let’s think about how users understand period and node materials. We initially saw period materials as a replacement for reusable materials (as discussed in https://github.com/ecamp/ecamp3/discussions/574). However, considering the feedback in #4280, we might need better explanations or additional features for period materials.

Your solution you proposed in #4364 might lead to a more intuitive understanding of period/node materials, instead of just disabling or hiding the filter. What are your thoughts on this?

@MarcoAllenspach
Copy link
Contributor Author

MarcoAllenspach commented Jan 1, 2024

For the activity view, I still believe removing the filter button completely would be best. It simplifies the UI and avoids confusion.
Ah now I have understood you...it's done on devel...MarcoAllenspach:ecamp3:fixedHideMaterialFilter
What you think showFilterButton is a good name or you have a better naming....?

I realy like the idea from #4280 but i but I understand that is a complex problem...
I was thinking period material is just someting you need in the camp but not on a specific activity....

#4364 is for me just an optimization that you can't apply a filter to something that doesn't exist

@manuelmeister
Copy link
Member

I think I would prefer a boolean attribute (meaning that the default value is false), so for instance disablePeriodFilter

Could you please either push the commits to the branch of this PR or create a new PR based on your new branch? This way the review can be added directly to the code.

I would rename the showFilter computed prop as well as it only disables the period filtering (e.g. periodFilterEnabled). Additionally the computed property should not have side effects (modifying state, in this case setting periodOnly to false).

github-merge-queue bot pushed a commit that referenced this pull request Jan 3, 2024
Hide Materialfilter PeriodOnly in ContentNode #4361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide Materialfilter PeriodOnly in ContentNode
2 participants