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

Use bigger font for menu on openvario-7-AM070-DS2 #121

Merged
merged 8 commits into from
Feb 20, 2021
Merged

Use bigger font for menu on openvario-7-AM070-DS2 #121

merged 8 commits into from
Feb 20, 2021

Conversation

Blaubart
Copy link
Contributor

@Blaubart Blaubart commented Feb 14, 2021

Closes #114

@DanD222
Copy link
Contributor

DanD222 commented Feb 17, 2021

I’m not a code reviewer, but here’s what’s anyone reviewing this pull request going to be facing:

  • The PR title is “solution to fox issue Missing font #114”. Issue 114 is about a missing font, but there are 3 commits here, and only one of them has anything to do with a font. It looks like the other two commits are related to Include the new adapter board DS2 and remove machine openvario-7-AM070 #119. Whoever’s looking at this PR is going to be scratching their head about what you’re trying to do here.

  • Your commits should really be called more descriptively, that is, not “resolve conflict”. A commit resolving a conflict or addressing an issue should be squashed with the commit where the issue was, so that you’re presenting just one commit for review (not one commit with a problem and another commit with a fix to the problem).

  • Calling a PR just “solution to fix something” forces a reviewer to switch back and forth between two windows to check what the problem was and whether you’re addressing it. What the problem is and how you’re addressing it should be in the commit message.

Here’s how to do it:

  1. Don’t use the web interface to make commits or to bring your branch up to date. You can’t squash commits using the web interface. Use an IDE with git integration, such as IntellJ IDEA, or a git GUI client, such as Git Kraken (what I use). Clone your repo, make your changes offline, squash commits if necessary, push to your branch (force push to overwrite your online repo so you don’t end up with a million “add this / delete that” commits). The neat thing is that if you have an open pull request, a force-push will update your PR with the changes you’ve made. Have a look at how people do it in successful pull requests in XCSoar.

  2. In case the main repo has moved forward compared to your fork, rebase your offline repo on the main repo. Don’t merge, merge commits don’t belong in a project’s code history. You can then push your rebased offline repo to your origin / online repo to bring it up to date with the main repo.

@Blaubart
Copy link
Contributor Author

Hi Dan,

thanks for your hints! Is it better too close this PR and open a new one or should it stay open?

@DanD222
Copy link
Contributor

DanD222 commented Feb 17, 2021

Hi Dirk,

I would say stick with this one and practice getting it into shape - it's perfect learning material actually.

Try it with Git Kraken - clone your repo, add the main Openvario repo as a Remote.

Make sure you’re checked out on your local warrior branch, then right click on remote/warrior and select “Rebase warrior onto Openvario/warrior” - this will show up any merge conflicts which you’ll need to resolve. Then try to figure out how to make your changes on your local repo and once you have have a single, properly titled and commented commit, do a Push to origin/warrior (your online repo) and this PR should get updated.

@Blaubart
Copy link
Contributor Author

Thanks for helping!!
I could solve conflict now.

@DanD222
Copy link
Contributor

DanD222 commented Feb 17, 2021

OK, now try to squash the 4 commits into one, so that there’s only one commit called “Add font to ovmenu-ng.sh” in the pull request (“Update ovmenu-ng.sh” is a pretty generic title that doesn’t really tell you what you’re trying to update).

@Blaubart
Copy link
Contributor Author

Blaubart commented Feb 17, 2021

Hi Dan,

sorry, I searched for a long time how to squash commits using GitHub Desktop. I couldn't find anything. The only thing I found was how it works with GitKraken and terminal.

Blaubart and others added 2 commits February 17, 2021 21:21
…mits.

resolve conflict

changed names of new machines

Update ovmenu-ng.sh
@kedder kedder changed the title solution to fix issue #114 Use bigger font for menu on openvario-7-AM070-DS2 Feb 17, 2021
Copy link
Member

@kedder kedder left a comment

Choose a reason for hiding this comment

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

@Blaubart, this looks good to me, but would you also apply this to CH070 variant, as requested in the #114?

@Blaubart
Copy link
Contributor Author

Hi Andrey,

thanks! I think the problem with this missing font is only with the Ampire Display because of the big resolution. Or do you think I should replace the missing font for the other displays with another one, too?

@kedder
Copy link
Member

kedder commented Feb 17, 2021

@Blaubart I was just reading the original issue on #114 and @glidero mentions both CH070 and PQ070 there. I don't think it makes sense to change the font on smaller screens, because this font might look way too big there. CH070 might benefit from the bigger font though. I think I've seen "font is too small" complaint on the forum before too.

@Blaubart
Copy link
Contributor Author

OK, at the moment I have no device with the CH070 ready to use. But I build one right now. So I can change it for this display too and will give a try after compiling. But I think I need a couple days to finish my OV.

@kedder
Copy link
Member

kedder commented Feb 17, 2021

@Blaubart I have CH070 at hand and I just tested the font before writing the previous response. It looks ok to me.

Copy link
Member

@kedder kedder left a comment

Choose a reason for hiding this comment

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

LGTM! Recommend merging with "Squash and Merge" option.

@linuxianer99 linuxianer99 merged commit de9ba7d into Openvario:warrior Feb 20, 2021
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.

Missing font
4 participants