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

New material view (subset) #3797

Merged
merged 14 commits into from
Sep 27, 2023

Conversation

manuelmeister
Copy link
Member

@manuelmeister manuelmeister commented Sep 17, 2023

Fixes subset of #3549

  • Sidebar
  • Keeps expansion panels for periods
  • Download each material list
  • Mobile optimization (without textfield modification)

@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Sep 17, 2023
@manuelmeister manuelmeister temporarily deployed to feature-branch September 17, 2023 21:36 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Sep 17, 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 manuelmeister temporarily deployed to feature-branch September 17, 2023 22:17 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 18, 2023 06:46 — with GitHub Actions Inactive
Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

Visually looks good to me :)
I think it is not very useful to export the material excels from all periods at once. But it was this way before, you didn't make it worse :)

api/src/Entity/ContentNode.php Outdated Show resolved Hide resolved
api/src/Entity/MaterialItem.php Outdated Show resolved Hide resolved
api/src/Entity/MaterialList.php Show resolved Hide resolved
frontend/src/views/material/MaterialOverview.vue Outdated Show resolved Hide resolved
e2e/specs/nuxtPrint.cy.js Show resolved Hide resolved
frontend/src/components/prompt/PopoverPrompt.vue Outdated Show resolved Hide resolved
frontend/src/router.js Outdated Show resolved Hide resolved
frontend/src/scss/global.scss Show resolved Hide resolved
frontend/src/views/material/MaterialOverview.vue Outdated Show resolved Hide resolved
@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 15:42 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 16:42 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 17:04 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 20:34 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 22, 2023 15:58 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 22, 2023 16:12 — with GitHub Actions Inactive
Copy link
Member

@usu usu left a comment

Choose a reason for hiding this comment

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

Look & feel good to me (except the point with the show/hide activity material, which I'd like to throw into the round)

Some optimizations on number of API calls still possible, but we can address this separately

api/src/Doctrine/Filter/ContentNodePeriodFilter.php Outdated Show resolved Hide resolved
api/src/Entity/ContentNode.php Outdated Show resolved Hide resolved
api/src/Entity/MaterialItem.php Outdated Show resolved Hide resolved
</template>
<v-card-text>
<e-switch
Copy link
Member

Choose a reason for hiding this comment

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

From a look & feel, removing the switch to hide activity material is the only thing I would challenge. The number of material items coming from activities could become very large for longer camps. Not having the possibility to hide activity material could make it very cumbersome, if I really want to review the camp/period material only.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the use case for this is. I think we should look at this in a separate issue, where we can tackle the confusion about period/activity material as well. For me this is the much bigger problem. Now you can just look at the individual lists which will alleviate the issue with loading.

If you think we should look at this in this issue, then I would suggest making the lastColumn sortable (based on type: activity/ period)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the use case for this is.

Let's take a 2 week camp. This could easily include a few hundred activity material items (14 days * 5 activities * 5 material items = 350 items). Now I want to add/edit my period/camp material (a few items only). In order to so so, i need to

  • Load all these other 350 material items
  • Visually navigate around these 350 material items which I'm not interested in. This is made even harder because at the moment, all period material is sorted at the beginning, but the form to add new items is at the bottom.

I'm not saying the hide/show toggle is the best and only solution. But above user experience is not very nice.

I'm also fine if we tackle this in a separate issue. However, in this PR we're removing code that allows to show/hide activity material which we might add again if we decide show/hide actually is the best solution. So I wonder if it's not easier to keep this toggle in at the moment (even if it's visually not very nice), and remove it later once we have a better solution for it.

Copy link
Member Author

@manuelmeister manuelmeister Sep 24, 2023

Choose a reason for hiding this comment

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

What do you think about: 0780683?

Copy link
Member Author

Choose a reason for hiding this comment

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

@usu I added a column header filter:
Unfiltered: Material list with "Activity / Period" column header and a outlined filter icon

Filtered: Material list with blue "Filter: Period" column header and a blue bold filter icon

Copy link
Member

Choose a reason for hiding this comment

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

😃 The functionality is there. The UX I'm not sure. But I think this is easier to discuss face-to-face + maybe trying to get some user feedback on how they use or want to use the material feature.

Copy link
Member Author

@manuelmeister manuelmeister Sep 26, 2023

Choose a reason for hiding this comment

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

Okay, maybe it is really better to discuss this at the hackathon. I still don't get the importance and think this whole mental model period vs activity material isn't helping our users. It has a bad UX and solves the wrong problem.

frontend/src/views/material/MaterialOverview.vue Outdated Show resolved Hide resolved
@manuelmeister manuelmeister added deploy! Creates a feature branch deployment for this PR and removed deploy! Creates a feature branch deployment for this PR labels Sep 23, 2023
@manuelmeister manuelmeister temporarily deployed to feature-branch September 24, 2023 20:06 — with GitHub Actions Inactive
@manuelmeister
Copy link
Member Author

  • Download link for xlsx doesn't work

Sorry this was due to a review suggestion that did not work.

  • Download link for xlsx is less prominent now - I hope it will be found

It is not the primary action. I tested this with someone and they expected it to be behind the menu.
Let's see if people can't find it.

  • URL when clicking on a list still contains the query part 'isDetail' (not necessary anymore, right?)

It is necessary, as when you click on mobile in the MaterialLists component (same as sidebar) then you'd want a back button.

@manuelmeister
Copy link
Member Author

@pmattmann could you create issues for the other suggestions? Then you could specify it a bit more, because I don't fully understand what you mean.

@manuelmeister manuelmeister dismissed stale reviews from carlobeltrame and pmattmann September 24, 2023 20:22

Implemented changes

@manuelmeister manuelmeister temporarily deployed to feature-branch September 24, 2023 20:34 — with GitHub Actions Inactive
@manuelmeister
Copy link
Member Author

manuelmeister commented Sep 24, 2023

I think this PR is ready to be merged as is functionality wise, as it doesn't get any easier to maintain. Could you please create further issues for feature requests with detailed descriptions.

Of course, bugs (like the download xlsx) are still a blocker, but everything else should be put into an enhancement issue and linked to this PR.

@manuelmeister manuelmeister added this to the MVP (v3.0) milestone Sep 24, 2023
@manuelmeister manuelmeister temporarily deployed to feature-branch September 25, 2023 11:23 — with GitHub Actions Inactive
@manuelmeister manuelmeister temporarily deployed to feature-branch September 25, 2023 11:24 — with GitHub Actions Inactive
@usu
Copy link
Member

usu commented Sep 26, 2023

I know you guys have different opinions on this, but I just wanted to reiterate that mixing PR reviews with rebasing and adding commit makes it really difficult to do proper delta reviews. I just spent way too much time, trying to figure out which code lines have changed since my last review.
Or I'm doing it wrong, happy to see tips/tricks how to do it more efficiently.

@manuelmeister
Copy link
Member Author

@usu as a reviewer I agree 😅
It is just more difficult to split the review changes and keep track of the fixups.
My problem is, that our PR review process is quite long and I don't want 5 merge commits in between.
Maybe I still don't fully get git. 😁

@manuelmeister manuelmeister added this pull request to the merge queue Sep 27, 2023
Merged via the queue into ecamp:devel with commit 85c3e25 Sep 27, 2023
28 checks passed
@manuelmeister manuelmeister deleted the improve/material-only-subset branch September 27, 2023 16:20
@carlobeltrame carlobeltrame mentioned this pull request Jan 12, 2024
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 type: Frontend UX/UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants