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

Text processing integration #4627

Merged
merged 4 commits into from
Aug 9, 2023
Merged

Text processing integration #4627

merged 4 commits into from
Aug 9, 2023

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Aug 4, 2023

📝 Summary

🚧 TODO

Follow ups

  • use notifications for triggering fetch instead of polling
  • FAB on mobile
  • How to handle markdown vs plain text
  • Dismiss notifications
  • Maybe later
    • merge translate to it and just move the actions to popover directly
    • Think about the making it show/hide depending on the mouse cursor (gdocs)

🖼️ Screenshots

Screenshot 2023-08-08 at 22 58 07 Screenshot 2023-08-08 at 22 57 47

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Aug 4, 2023

Passing run #11609 ↗︎

0 149 2 0 Flakiness 0

Details:

Text processing integration
Project: Text Commit: 5ebcd8bd77
Status: Passed Duration: 04:15 💡
Started: Aug 9, 2023 4:18 PM Ended: Aug 9, 2023 4:22 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@juliusknorr juliusknorr force-pushed the feat/text-processing branch 3 times, most recently from 6387d31 to 525faf5 Compare August 8, 2023 21:00
@juliusknorr juliusknorr changed the title feat/text processing Text processing integration Aug 8, 2023
@juliusknorr juliusknorr marked this pull request as ready for review August 8, 2023 21:02
@juliusknorr juliusknorr added enhancement New feature or request 3. to review labels Aug 8, 2023
@juliusknorr juliusknorr added this to the Nextcloud 28 milestone Aug 8, 2023
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks really great! :) The only thing I am a bit worried about is that even with the icon on the button, "Show assistant results" in the menu then is not super obvious.

  • Should we permanently show "Assistant results" as an entry on the top instead?
  • We could also say "Assistant results (4 new)" or have a counter bubble there for unviewed results if possible?
  • Other ideas?

@nimishavijay
Copy link
Member

Looks really nice! Super cool! Only one thing: maybe we can add a separator line before "Show assistant results" to indicate a different type of action. Other than that it looks great! :)

@juliusknorr
Copy link
Member Author

Fixed:

Screenshot 2023-08-09 at 14 07 04

@juliusknorr
Copy link
Member Author

Looks really great! :) The only thing I am a bit worried about is that even with the icon on the button, "Show assistant results" in the menu then is not super obvious.
Should we permanently show "Assistant results" as an entry on the top instead?
We could also say "Assistant results (4 new)" or have a counter bubble there for unviewed results if possible?
Other ideas?

I think the indicator about the status already gives an idea, considering the timeline of getting this merged, I'd rather get it in and align during the demo next week if this is something we can polish next week and delay screenshot taking or keep as it is. In any case having clear mockups for any permanent display option would be appreciated then.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Code looks good. Did not try myself.

I think I'd personally compose the Assistent.vue out of smaller components for the Floating Menu and the Modal. But that can also happen later. I also did not check how much code reuse there is between the two. If they overlap a lot might not be worth it either.

@juliusknorr
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <[email protected]>
@juliusknorr juliusknorr merged commit 67bc06e into main Aug 9, 2023
40 checks passed
@juliusknorr juliusknorr deleted the feat/text-processing branch August 9, 2023 16:43
@juliusknorr
Copy link
Member Author

/backport to stable27

@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@juliusknorr
Copy link
Member Author

/backport 526302d,130a4ec5fe5f24e0e27a63e459eeea22472770e5,5ebcd8bd7788afa300b2da098a88067b59eec9dc to stable27

@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate text processing API
5 participants