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

Make la thematic area content node more compact #4264

Merged
merged 9 commits into from
Dec 29, 2023

Conversation

manuelmeister
Copy link
Member

Improve the ContentNode to be more dense and improve error messages.

Bildschirmfoto 2023-12-18 um 13 16 00

Bildschirmfoto 2023-12-18 um 16 54 25

@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Dec 18, 2023
Copy link

github-actions bot commented Dec 18, 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.

@BacLuc

This comment was marked as off-topic.

@BacLuc
Copy link
Contributor

BacLuc commented Dec 19, 2023

It doesn't seem to work as intended:

But also not on dev:

but this is broken on staging (and i assume on prod) too

uups, i detected another bug

@BacLuc
Copy link
Contributor

BacLuc commented Dec 19, 2023

i am not yet convinced that it is good to show the available items >100 pixels below the trigger point.
Maybe a toggle to hide/show the not selected items, default is hidden?

@manuelmeister
Copy link
Member Author

manuelmeister commented Dec 19, 2023

@BacLuc sorry I'm not sure what you are talking about. I'm fixing the bug for notes, safetyconcept & storycontext in a separate bugfix #4268

@usu
Copy link
Member

usu commented Dec 22, 2023

I like the look of the compact style.

A few issues with the behavior though:

  • Selecting new entries is buggy. See video below.

  • Animation is not consistent with other similar inputs. For example on Activity Responsibles, newly selected items from the dropdown are added immediately to the main input & an animation shows that saving is in progress and other animation when the saving was successful. This is very consistent with other inputs. In your current proposal for LA, you deviate from this: New selected items ar

Recording.2023-12-22.102005.mp4

e only added to the main input after successful saving. There's no animation indicating a save-in-progress.

…-activity

# Conflicts:
#	frontend/src/components/activity/CardContentNode.vue
#	frontend/src/components/activity/content/LAThematicArea.vue
@manuelmeister
Copy link
Member Author

@usu thanks for the review. You're right. I adjusted the functionality and fixed the buggy behaviour.

@usu
Copy link
Member

usu commented Dec 23, 2023

Nice! Last thing I noticed is that there is no dirty handling (see video below). Selected keys is overriden, whenever a API response comes back.

Recording.2023-12-23.094734.mp4

@manuelmeister
Copy link
Member Author

@usu how could I solve this?

@usu
Copy link
Member

usu commented Dec 25, 2023

@usu how could I solve this?

The main principle could be copied from ActivityResponsibles.vue:

You'd want a local copy of the selected items (selectedCampCollaborations) which is used as the v-model for e-select. Then you need a watcher on the store/API value (currentCampCollaborationIRIs). In this watcher you'd check if the incoming selection (newIRIs) is the same as the local copy (selectedCampCollaborations). If yes, the dirty flag is removed.

  • You can give it a try. Or I can make a proposal. As you prefer...
  • Maybe some of the logic could be abstracted into a common component or a composable to avoid code duplication. But we could also do this in a second step, if we see it is worth.
  • Above dirty logic has 1 flaw: If you quickly change back-and forth between the same selection, at one point the dirty flag is removed, even though more network requests are in progress. A better variant would be to use a dirtyTimestamp to ensure, only the last network request can remove the dirty flag. We currently use this improved version only for dragging content nodes (RootNode, DraggableContentNodes), so I think it would be fair to just implement the simplified variant.

@manuelmeister
Copy link
Member Author

manuelmeister commented Dec 26, 2023

@usu thanks for the help! I've implemented the dirty handling while keeping the debounced save, because the individual options are not saved in separate db fields, we try to limit the requests.

Because the select can grow high, it can be confusing if the overlay is out of view.
@manuelmeister
Copy link
Member Author

@BacLuc I fixed your issue. The overlay is now on top of the select like in the other instances.

Copy link
Contributor

@BacLuc BacLuc left a comment

Choose a reason for hiding this comment

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

With the fix of the requestCount decrease in the error case, this is ok for me.
Thank you

@manuelmeister manuelmeister added this pull request to the merge queue Dec 29, 2023
Merged via the queue into ecamp:devel with commit 92562d1 Dec 29, 2023
32 checks passed
@manuelmeister manuelmeister deleted the feature/compact-la-activity branch December 29, 2023 12:57
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.

3 participants