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.

Sign up for GitHub

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

Jump to bottom

Fix OSD preview for Android #3221

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Fix OSD preview for Android #3221

merged 1 commit into from
Jan 10, 2023

Conversation

haslinghuis
Copy link
Member

No description provided.

@haslinghuis haslinghuis added this to the 10.9.0 milestone Jan 7, 2023
@haslinghuis haslinghuis self-assigned this Jan 7, 2023
@haslinghuis haslinghuis marked this pull request as draft January 7, 2023 01:57
@haslinghuis
Copy link
Member Author

@McGiverGim artifacts workflow seems out of date. Perhaps we replace with https://github.com/actions/download-artifact

@McGiverGim
Copy link
Member

@McGiverGim artifacts workflow seems out of date. Perhaps we replace with https://github.com/actions/download-artifact

I don't understand exactly what you say, we are using these action. What action must we replace?

@haslinghuis
Copy link
Member Author

I mean the artifacts have no link in the PR. (Now need to go to show all checks)

@McGiverGim
Copy link
Member

This happens for draft.

@haslinghuis haslinghuis force-pushed the fix-osd-android branch 5 times, most recently from f50332c to 703711d Compare January 8, 2023 16:46
@haslinghuis haslinghuis marked this pull request as ready for review January 8, 2023 16:46
@haslinghuis haslinghuis force-pushed the fix-osd-android branch 2 times, most recently from 1f5214b to 9d03a8c Compare January 8, 2023 17:23
blckmn
blckmn requested changes Jan 8, 2023
View reviewed changes
Copy link
Member

@blckmn blckmn left a comment

Choose a reason for hiding this comment

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

@haslinghuis id prefer to delete than comment. You can always revert based on file history.

Risk is that the commented code stays, and gets forgotten.

@haslinghuis
Copy link
Member Author

Yes lazy to setup local android dev, testing display local on desktop and using the build here to test on android. Will cleanup when finished.

@haslinghuis haslinghuis force-pushed the fix-osd-android branch 4 times, most recently from cbc9022 to 91b4903 Compare January 8, 2023 20:57
@McGiverGim
Copy link
Member

@haslinghuis seems here is a workaround tonyhallett/artifacts-url-comments#80

I'm out of the computer until tomorrow, so I can't do the change right now.

@haslinghuis
Copy link
Member Author

OSD preview looks good on Nokia G60. Happy if you could test with your Pixel device.

@haslinghuis
Copy link
Member Author

Will do a PR for

Update: The issue is in the addTo: "pullandissues" if I only use addTo: "pull" everything works as expected

@McGiverGim
Copy link
Member

OSD preview looks good on Nokia G60. Happy if you could test with your Pixel device.

Looks as good as possible in a phone (tested with virtual fc). I miss a little margin to the right of the preview, but for the rest is ok.

@haslinghuis
Copy link
Member Author

How to get the assets for testing this PR:

There are no artifacts published so please click Checks tab and click PR to find the artifacts

@haslinghuis
Copy link
Member Author

@McGiverGim for me it shows preview on top and the other columns next to each other on the bottom. No problem with margin :)

I think this is much better as before.

@McGiverGim
Copy link
Member

I'm talking about this left space:
Uploading Screenshot_20230108-230354.png…

On the right there is none:
Uploading Screenshot_20230108-230330.png…

@McGiverGim
Copy link
Member

Screenshot_20230108-230330
Screenshot_20230108-230354

@haslinghuis
Copy link
Member Author

At least we don't have to scroll to the right anymore to see the preview :) and it's not cutoff anymore.
Tried to minimize fixed values as much as possible. For me it looks good and on Pixel good enough.

@McGiverGim
Copy link
Member

I scroll to the right because the preview does not fit in the page 😁

@haslinghuis
Copy link
Member Author

haslinghuis commented Jan 8, 2023 •
edited
Loading

Ok adding 528 max-width: 100% too. Let's see.

Perhaps we need to add to 515 instead.

@haslinghuis
Copy link
Member Author

@McGiverGim please test again

@McGiverGim
Copy link
Member

I can't see any difference, but it's ok to me. 😉

@haslinghuis
Copy link
Member Author

Ok, reverted as it didn't work.

Good to merge now.

@sonarcloud
Copy link

sonarcloud bot commented Jan 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@blckmn
Copy link
Member

blckmn commented Jan 9, 2023 •
edited
Loading

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@haslinghuis haslinghuis merged commit 0404207 into betaflight:master Jan 10, 2023
@haslinghuis haslinghuis deleted the fix-osd-android branch January 10, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@McGiverGim McGiverGim McGiverGim approved these changes

@blckmn blckmn blckmn approved these changes

Assignees

@haslinghuis haslinghuis

Labels
RN: UI
Projects
None yet
Milestone
10.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@haslinghuis @McGiverGim @blckmn