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

Fix ove and issues with uploadsDiv and the observer #4867

Merged
merged 7 commits into from
Jan 20, 2024

Conversation

MarcelBolten
Copy link
Contributor

Hi Nico,
The fix for ove was rather easy. Turns out ove is now adding messages also in case of successfully parsing a file.
But the rest of this PR was a bit tedious. It looks like you have been there recently as well (#4843).
We better don't observe the subtree!
I undid stuff that is related to:

// this exists here because in the Observer of uploads.ts for filesdiv it makes the browser crash
// FIXME

But maybe this needs a bit more cleanup? What do you think?

closes #4863

MarcelBolten and others added 3 commits January 19, 2024 01:16
don't observe the subtree
* hypernext:
  add a notification when new version has been updated after login
  add configurable chat room link
  add emit_audit_logs instance config to emit audit logs as NOTICE in php
  add import/export audit log event
  try and fix issue with multiple data-trigger listeners on the page
  add requester info to Users2Teams class
@NicolasCARPi
Copy link
Contributor

Hello Marcel,

Thank you for this. It seems the button to toggle display mode of uploads isn't working properly anymore, and I see an error from autoload.ts of 3dmol:

2024-01-19-124649_646x107_scrot

So this probably needs more work!

@NicolasCARPi
Copy link
Contributor

So the toggle display button becomes functional again if the reloadElement call targets filesdiv instead of uploadsDiv. So a reloadUploads() function that reloads properly this part of the page would still be useful IMHO.

@MarcelBolten
Copy link
Contributor Author

It seems the button to toggle display mode of uploads isn't working properly anymore

The toggle button is fixed now.

I see an error from autoload.ts of 3dmol

I could not reproduce this error. Let me know if that is still an issue. Works for me with Firefox.

So the toggle display button becomes functional again if the reloadElement call targets filesdiv instead of uploadsDiv. So a reloadUploads() function that reloads properly this part of the page would still be useful IMHO.

The problem is that the mutation observer will run into an infinite loop if both options (childList and subtree) are true as we have some tools that heavily modify the dom as you have realized #4843.
Only using childList does not work on filesdiv because with the changed layout the children of uploadsDiv are changed.
So we must change the observer to uploadsDiv. But than we need to reload the toggle display button in addition.
Also the singular/plural switch of uploads will need a page reload now. There will be someone at some point that will open an issue about that but for now I care more about the overall functionality.

For chemdoodle to work with the changes made here, PR deltablot/chemdoodle-web-mini#2 is necessary.

@MarcelBolten MarcelBolten marked this pull request as ready for review January 20, 2024 00:48
@NicolasCARPi
Copy link
Contributor

Try and add a .pdb such as https://raw.githubusercontent.com/NicolasCARPi/example-files/master/example.pdb

Then reload the page:
2024-01-20-132944_1758x799_scrot

It looks like it's trying to load the file from rcsb.org for some reason....

@NicolasCARPi
Copy link
Contributor

Removing the first autload call seems to fix this pdb issue...

Copy link
Contributor

@NicolasCARPi NicolasCARPi left a comment

Choose a reason for hiding this comment

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

I'll wait for a final ack from you before merging because I pushed changes, but LGTM!

@MarcelBolten
Copy link
Contributor Author

It looks like it's trying to load the file from rcsb.org for some reason....

We have data attributes that are conflicting with 3Dmol. Compare to their docs https://3dmol.org/doc/tutorial-embeddable.html
In particular,data-href will result in 3Dmol to try load the file. Would have been nice if they had added a namespace to their data attributes.

@NicolasCARPi NicolasCARPi merged commit f51dc26 into hypernext Jan 20, 2024
8 of 9 checks passed
@NicolasCARPi NicolasCARPi deleted the marcel-fix-ove branch January 20, 2024 16:19
NicolasCARPi added a commit that referenced this pull request Jan 20, 2024
* hypernext:
  fix required label not appearing on all elements
  Fix ove and issues with uploadsDiv and the observer (#4867)
  Avoid casting in favor of getString and get Int; harmonization of the use of App->Request (#4872)
  fix drag and drop issues when uploads in table display mode
  add sysconfig setting to prevent admins from archiving users
  add a notification when new version has been updated after login
  add configurable chat room link
  add emit_audit_logs instance config to emit audit logs as NOTICE in php
  add import/export audit log event
  try and fix issue with multiple data-trigger listeners on the page
  add requester info to Users2Teams class
  simplify function notation
  don't disable the timestamp button on click
  always display main text + extra fields in toggle body in show mode
  add custom_id to CSV export
  add owner parameter for GET all
  add diff syntax highlighting
  add `metadata_decoded` to JSON output for single entity
  Fix the tinyMCE mention plugin (#4852)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants