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

Add open action for editors #11085

Merged
merged 21 commits into from
Jul 4, 2024
Merged

Add open action for editors #11085

merged 21 commits into from
Jul 4, 2024

Conversation

AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Jun 21, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests
  • Documentation
  • Maintenance (e.g. dependency updates or tooling)

Open tasks:

  • ...

Copy link

update-docs bot commented Jun 21, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented Jun 22, 2024

TBD:

  • Fix for shares ✅
  • Display disabled mode in tiles view ✅
  • Make actionable in tiles view ✅
  • Make work with external apps and preview app
  • Fix as type any conversions ✅ ** known issue*
  • Add documentation ✅
  • Add tests

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Nice progress! I noticed some things:

  • if I have the right sidebar open in the main window, the embed mode in the modal starts with visible right sidebar. I'd like the embed mode to always start with closed sidebar
  • paddings are somehow off in the right sidebar. E.g. the x button for closing the right sidebar floats out of the sidebar, into the blue border on the right.
  • visually it makes a quite heavy impression, that the embed mode has the dark blue frame and is placed in an otherwise white modal.

Functionality already works flawlessly 💯

@AlexAndBear
Copy link
Contributor Author

AlexAndBear commented Jun 24, 2024

Nice progress! I noticed some things:

  • if I have the right sidebar open in the main window, the embed mode in the modal starts with visible right sidebar. I'd like the embed mode to always start with closed sidebar

Since the sidebar will be controlled by localStorage, this is a little hard to achieve, maybe in a followup

  • paddings are somehow off in the right sidebar. E.g. the x button for closing the right sidebar floats out of the sidebar, into the blue border on the right.

Not reproducible, could you check again and upload a screenshot?

  • visually it makes a quite heavy impression, that the embed mode has the dark blue frame and is placed in an otherwise white modal.

@tbsbdr remarked this as well, and proposed a blue modal, @kulmann is against that, can you please propose something you both are fine with?

Functionality already works flawlessly 💯

:)

@AlexAndBear AlexAndBear marked this pull request as ready for review June 26, 2024 11:09
@CLAassistant
Copy link

CLAassistant commented Jun 27, 2024

CLA assistant check
All committers have signed the CLA.

@AlexAndBear AlexAndBear requested a review from kulmann June 27, 2024 21:00
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Only a few tiny remarks, otherwise awesome stuff!

Did you try what happens if token renewal takes places during the modal being open? I noticed the iFrame spawns another web worker for the token renewal... I hope it doesn't have a negative effect 🙈

@AlexAndBear AlexAndBear requested a review from JammingBen July 2, 2024 16:12
@AlexAndBear
Copy link
Contributor Author

Only a few tiny remarks, otherwise awesome stuff!

Did you try what happens if token renewal takes places during the modal being open? I noticed the iFrame spawns another web worker for the token renewal... I hope it doesn't have a negative effect 🙈

I didn't 🙈

@AlexAndBear
Copy link
Contributor Author

Changelog is missing

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

This is working perfectly, so I'm just gonna go ahead and approve this.

For the visuals I have one proposal though: could you drop the modal footer and instead of the cancel button add an X in the top right of the modal for closing the modal? That could/should look like the following screenshot and IMO that makes it look much better already, because the white frame at the bottom disappears. Still not happy with the header section, but IMO this would be an improvement.

Screenshot 2024-07-04 at 10 01 07

@AlexAndBear
Copy link
Contributor Author

@kulmann Unfortunately I can't easily, we use the dispatchModal function here to display the modal, and our FilePickerModal component will be just the rendered content. I only have the possibility to adjust CSS but injecting elements would end up in spaghetti code.

Another possibility could be:
Add the cancel button to the bottom like we have it in other file picker modes and just disable the header and top of the modal via CSS

@kulmann
Copy link
Member

kulmann commented Jul 4, 2024

Another possibility could be: Add the cancel button to the bottom like we have it in other file picker modes and just disable the header and top of the modal via CSS

Yes, sounds good. But as a followup please. :-)

Copy link

sonarqubecloud bot commented Jul 4, 2024

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Noice 👍

@AlexAndBear
Copy link
Contributor Author

Another possibility could be: Add the cancel button to the bottom like we have it in other file picker modes and just disable the header and top of the modal via CSS

Yes, sounds good. But as a followup please. :-)

I'll merge this and open a follow up pr, so we can show it tomorrow

Toodalo!

@AlexAndBear AlexAndBear enabled auto-merge (squash) July 4, 2024 08:19
@AlexAndBear AlexAndBear merged commit fd350b8 into master Jul 4, 2024
3 checks passed
ownclouders pushed a commit that referenced this pull request Jul 4, 2024
* Add open action for  editors

Co-authored-by: Jannik Stehle <[email protected]>
@micbar micbar mentioned this pull request Jul 8, 2024
19 tasks
@kulmann kulmann deleted the open-in-app-action branch September 5, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Web] Open file from within tab bar
4 participants