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 messages in 'Note To Self' chat room take full available screen width #4408

Conversation

arkascha
Copy link
Contributor

@arkascha arkascha commented Oct 31, 2024

🖼️ Screenshots

🏚️ Before 🏡 After
screenshot_befe screenshot_after

DESCRIPTION

This PR implements the change requested in issue #3920.

Messages bubbles in "NoteToSelf" chat room use the full available width now. There is no sense in leaving a start margin, since no other participant can add messages to that chat room.

🚧 TODO

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

@arkascha arkascha changed the title Fix issue 3920: messages in 'NoteToSelf' chat room take full available screen width now Make messages in 'Note To Self' chat room take full available screen width Nov 1, 2024
@arkascha
Copy link
Contributor Author

arkascha commented Nov 1, 2024

I fail to understand what the failed analysis is actually trying to say.
I do not see any detail reported by any of the checks that somehow corresponds to my changes.
Is there anything I miss? Anything I need to do, to fix?

@AndyScherzinger AndyScherzinger added the 3. to review Waiting for reviews label Nov 1, 2024
@mahibi
Copy link
Collaborator

mahibi commented Nov 5, 2024

I fail to understand what the failed analysis is actually trying to say. I do not see any detail reported by any of the checks that somehow corresponds to my changes. Is there anything I miss? Anything I need to do, to fix?

don't worry about the failed analysis.
Thank you for contributing! I will try it out tomorrow, code wise it looks good 👍

@mahibi mahibi enabled auto-merge November 6, 2024 09:16
@mahibi
Copy link
Collaborator

mahibi commented Nov 6, 2024

works fine 🥇
i enabled auto merge. could you rebase on newest master @arkascha ?

If you like to continue contributing, just let me know if you are interested to get access to talk android repo so you could create branches in it...

auto-merge was automatically disabled November 6, 2024 18:34

Head branch was pushed to by a user without write access

@arkascha arkascha force-pushed the issues-3920-note-to-self-messages-should-take-up-most-of-the-screen-width branch from 716d8e8 to 18228e1 Compare November 6, 2024 18:34
@arkascha arkascha force-pushed the issues-3920-note-to-self-messages-should-take-up-most-of-the-screen-width branch from 18228e1 to 238532f Compare November 6, 2024 18:55
@arkascha
Copy link
Contributor Author

arkascha commented Nov 6, 2024

@mahibi I rebased and fixed an issue pointed out by Codacy.
I did NOT squasch the two commits, I intentionally left the in the PR since they target different issues. I personally prefer to have such things separate in the git history in my own projects.
Auto merging does not seem to continue though, "This workflow requires approval from a maintainer. " is mentioned above.

@arkascha
Copy link
Contributor Author

arkascha commented Nov 6, 2024

@mahibi About access to your repository ... Thanks for the trust in me, but for now I am happy with working on my own clone, I think. As long as PRs like this one are OK for you. This PR was just a test balloon for some bigger idea I would like to try. No idea if I succeed, so I do not want to create any hassle.

@mahibi
Copy link
Collaborator

mahibi commented Nov 7, 2024

@mahibi About access to your repository ... Thanks for the trust in me, but for now I am happy with working on my own clone, I think. As long as PRs like this one are OK for you. This PR was just a test balloon for some bigger idea I would like to try. No idea if I succeed, so I do not want to create any hassle.

Sure, working with the fork is okay for now.
Please feel free to share the bigger idea you have in mind (best by creating an issue). Just want to avoid that work is done and soon afterwards it's obsolete because some technology is replaced etc.
E.g. the ViewHolders will be affected/replaced when switching to Jetpack Compose etc..

@mahibi mahibi enabled auto-merge November 7, 2024 08:19
@arkascha
Copy link
Contributor Author

arkascha commented Nov 7, 2024

@mahibi There seems to be an issue again. 2 failed checks (which I fail to make much sense of) and one check still expected (which seems odd).

Signed-off-by: Andy Scherzinger <[email protected]>
@AndyScherzinger
Copy link
Member

@arkascha I bumped the detekt score by 1 (this PR add another detekt warning / issue) yet it is hard spot spot which one is the new one since the report simply lists all finding. Not too much of an issue, hence I bumped the score, so detekt should now turn green.

@AndyScherzinger AndyScherzinger added the enhancement New feature or request label Nov 7, 2024
@arkascha
Copy link
Contributor Author

arkascha commented Nov 7, 2024

@AndyScherzinger , @mahibi Thanks, so now we are down to just one failed check. To me that one sounds like a broken setup of the tool chain. Anything I can do in this?

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Nov 7, 2024

A bit of both @arkascha the check failed to upload the result but also fails compared to master, see https://github.com/nextcloud/talk-android/actions/runs/11724053977/job/32657114027?pr=4408#step:6:100

I pushed a commit fixing some spotbugs issues to see/get to a score not higher in terms of number of warnings compared to master. If we are back at 108 warning (let's see what the check returns) than I can force merge 👍

@AndyScherzinger AndyScherzinger added this to the 20.1.0 milestone Nov 7, 2024
@AndyScherzinger
Copy link
Member

So.... Spotbugs is back at the same amount of warnings we have on master, hence everything is good to go.

Thanks a lot for this contribution @arkascha - highly appreciated and thank you for your patience in getitng everythign in order to get it to be merged and sorry for the (still) broken analysis Github action.

@AndyScherzinger AndyScherzinger merged commit 4b88136 into nextcloud:master Nov 7, 2024
16 of 17 checks passed
@arkascha
Copy link
Contributor Author

arkascha commented Nov 7, 2024

Thanks for helping out @AndyScherzinger!
Looking through the last commit you added I have the impression that none of those things you fixed are related to my changes. So I still wonder what failing checks I added with my changes...

Anyway, great that things are merged now. It has been a pleasure to contribute a tiny detail.
And thanks for the awesome app at all!

@arkascha
Copy link
Contributor Author

arkascha commented Nov 7, 2024

I assume the related issue can be closed now.
I do not have the required permission to do that, though.

@AndyScherzinger
Copy link
Member

Yes, thanks for the ping - closed the issue 👍

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

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

Successfully merging this pull request may close these issues.

4 participants