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

[f2v] Render inline system tags using new DAV properties API #40284

Merged
merged 5 commits into from Sep 13, 2023

Conversation

lhsazevedo
Copy link
Contributor

@lhsazevedo lhsazevedo commented Sep 6, 2023

Ref: #39914 #37938

Summary

Display inline system tags using the new DAV properties API.

image

image

I kept the action in the files app since the previous plugin was in there as well, and I didn't found any action registration outside the files* apps. But I can move it to the systemtags app if that is preferred!

TODO

  • Decide if we should move the action to the systemtags app

Checklist

@lhsazevedo
Copy link
Contributor Author

Possible follow ups:

  • Maybe we should hide the system tags (or all inline action?) on mobile to avoid truncating the node name?
    image
  • Looks like the search by tag feature needs to be migrated as well.

@skjnldsv

This comment was marked as resolved.

skjnldsv

This comment was marked as resolved.

@lhsazevedo
Copy link
Contributor Author

lhsazevedo commented Sep 6, 2023

Done! Added some tests as well. For now I'm hiding the tags on mobile using CSS media queries, as I'm not sure if registering an size observer inside renderInline would be the proper way to make an action react to a screen resize or if we should modify the FileAction API for this instead.

Unfortunately the tags aren't being rendered on the initial DAV request after moving to the systemtags app. I checked to ensure the action is being called, and it is indeed, but looks like now the dav property isn't being registered in time for the first request. Subsequent requests have the property registered:

inline-system-tags-bug.mp4

I believe it isn't possible to register the property earlier while still in the systemtags app, is it?

@skjnldsv
Copy link
Member

skjnldsv commented Sep 7, 2023

Done! Added some tests as well. For now I'm hiding the tags on mobile using CSS media queries, as I'm not sure if registering an size observer inside renderInline would be the proper way to make an action react to a screen resize or if we should modify the FileAction API for this instead.

Media queries is how it should be done.
If you wanna prevent the fetching for ressources optimisation, you can also use document.documentElement.clientWidth < 512 and not register the dav props? 🤔

Unfortunately the tags aren't being rendered on the initial DAV request after moving to the systemtags app. I checked to ensure the action is being called, and it is indeed, but looks like now the dav property isn't being registered in time for the first request. Subsequent requests have the property registered:

We actually discovered that with @juliushaertl yesterday and worked on a fix, you can ignore it :)

@skjnldsv skjnldsv mentioned this pull request Sep 7, 2023
26 tasks
@lhsazevedo
Copy link
Contributor Author

If you wanna prevent the fetching for ressources optimisation, you can also use document.documentElement.clientWidth < 512 and not register the dav props? 🤔

Not sure if the complexity trade off is worth it... We'll probably consume them in other places of the app soon, so I guess it's Ok for them to be available on mobile as well for now?

@skjnldsv skjnldsv added this to In progress in Files to vue via automation Sep 13, 2023
Signed-off-by: Lucas Azevedo <lhs_azevedo@hotmail.com>
Signed-off-by: Lucas Azevedo <lhs_azevedo@hotmail.com>
Signed-off-by: Lucas Azevedo <lhs_azevedo@hotmail.com>
Signed-off-by: Lucas Azevedo <lhs_azevedo@hotmail.com>
Signed-off-by: Lucas Azevedo <lhs_azevedo@hotmail.com>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 13, 2023
@skjnldsv skjnldsv merged commit 165c54d into nextcloud:master Sep 13, 2023
35 of 39 checks passed
Files to vue automation moved this from In progress to Done Sep 13, 2023
@lhsazevedo lhsazevedo deleted the inline-system-tags branch September 14, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: files feature: tags
Projects
Files to vue
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants