-
Notifications
You must be signed in to change notification settings - Fork 50
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 page #3549
New material page #3549
Conversation
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
9728170
to
5098217
Compare
5098217
to
cef88c1
Compare
type="number" may not be adequate for every number input, as it does not allow free textinput
This element disregards its min-width and instead adds ellipsis if the text is longer than the available space
cef88c1
to
f4df208
Compare
b126d9e
to
149a9e4
Compare
What is the problem with this print test? Can someone fix it, so it isn't flakey anymore. I already tried in #3526 but we had to roll back, because firefox did not like it. |
Did the tests run successfully on your local machine? |
In #3526 yes |
Some thoughts when trying it out on the deployment:
|
Haven't read the comments from @carlobeltrame by purpose, to not let me influence. With the risk, that some comments might be double.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the issue with the e2e tests
@carlobeltrame Thank you for your review. I tried answering all questions/remarks:
|
@usu thank you for your review. I tried answering all questions/remarks:
|
How should I continue with this PR? Do you agree with my suggestions, or should I try out something else first, before I cleanup and make it ready to be merged. |
Thanks for the thorough answers, I agree with practically everything. However, I don't agree to keep the table at all costs. The table is the main source of clunkiness for me, and a table in a UI is often a sign that devs have "just" recreated the database structure in UI. If a person would manually write a material list for their camp, I doubt they would start by drawing a table grid on the paper, they would write it more like a bullet list with tabstops after the quantity/unit (at least I would). On ecamp, I would also never want to filter or sort the list by quantity or unit. I would either sort alphabetically, or by activity/period, and filter by material list, nothing else. For the filtering, you already went for a different solution (sidebar) than how classic datatables would do it (dropdown menu in column headers). So if you offer any way to sort, e.g. via two buttons, I see no real reason for the list to be a table anymore. This is just a suggestion to try to experiment further, I'm not saying my proposal is perfect or even close to the solution. I'm also fine with merging this in the current state (after you fix what you wanted to fix above). It's just that this redesign PR hasn't triggered the "wow" effect for me, like your other redesign PRs have done. I don't quite see the redesigned material list as finished after this PR. But it sure is an improvement already. |
It looks good, thank you for the proposal.
I think we need to agree how we want to indicate the activity/period, and then you could make this PR merge ready. |
@@ -341,7 +381,7 @@ async function requireCamp(to, from, next) { | |||
await campFromRoute(to) | |||
.call({ api: { get: apiStore.get } }) | |||
._meta.load.then(() => { | |||
next({ query: to.query }) | |||
next() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this line (blame) was non sensical to begin with and written by me 🫣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! I see progress compared to the previous version.
Especially the possibility to display a specific material list makes it clearer.
A few thoughts from my side:
- The first overview of all lists has little additional value compared to when you are in a list. So you could jump directly to the 'General overview' view.
- I would also subdivide 'General overview' by periods.
- Excel export also for individual lists
- After adding an entry to a stock list, the field 'MaterialList' is no longer pre-populated.
- [Delete] and [Add to period] buttons are extremely large
- Maybe we can adjust the order of the columns:
- Item
- quantity
- Unit (default; Stk; in future maybe automatic guess depending on the article)
Core Meeting DiscussionCreate a reduced pr with the following subset, to fasttrack improvements:
|
Is there stuff worth keeping in here which wasn't covered in #3797? Or can we close this? |
@carlobeltrame I would like to keep it until I've implemented all improvements. |
referenced in an issue |
I've created a new version of the material list that is more optimized for the different uses. It is not yet published and should serve as an ground for discussion. I tried optimizing the wording and visual styling so it makes it easier to understand where the data lives. (Period / Material Node)
I'm not yet happy with the period material (our attempt to fix period material, aka "reusable material" from eCamp2). But I would point further discussion to the issue #1245
It is not optimized for speed and is not yet polished for merging. This should serve as POC for my new layout.
Relates to #3576