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

HRM Widget+ adds bpm digits in lato font, plus some README, icon, screenshot fixes #3018

Merged
merged 7 commits into from
Sep 22, 2023

Conversation

hughbarney
Copy link
Contributor

No description provided.

@bobrippling
Copy link
Collaborator

Nice, LGTM! Just two little things on intercepting setHRMPower

apps/widhrt2/widget.js Outdated Show resolved Hide resolved
apps/widhrt2/widget.js Outdated Show resolved Hide resolved
@bobrippling
Copy link
Collaborator

I'm happy to merge (and install on my watch!) if that's ok with you too @thyttan?

@thyttan
Copy link
Collaborator

thyttan commented Sep 17, 2023

I'm happy to merge (and install on my watch!) if that's ok with you too @thyttan?

I didn't look too closely - if you and @hughbarney are happy that's good enough for me 👍

I'd personally maybe have opted for the id to be widhrtplus to match the user facing name, like we did with runplus/Run+. But doesn't really matter :p

@bobrippling
Copy link
Collaborator

I'd personally maybe have opted for the id to be widhrtplus to match the user facing name, like we did with runplus/Run+

Yeah that might help it stand out more, what do you think @hughbarney?

@gfwilliams
Copy link
Member

I'd personally maybe have opted for the id to be widhrtplus

Yes, I think this would be best. No other apps use + in the name, and I'd be a bit concerned that it could cause issues trying to store those files on some operating systems?

But otherwise this looks good

@hughbarney
Copy link
Contributor Author

hughbarney commented Sep 18, 2023

I have changed id to widhrtplus. I should point out that no files have a + in the name.

I'm rather confused by how to manage edits to a pull request once it has been submitted; I might have broken other changes that Rob did to my pull request above, I have no idea how I would get such changes into my branch to merge them etc.

(@thyttan accidentally edited this comment - sorry!)

@thyttan
Copy link
Collaborator

thyttan commented Sep 19, 2023

I'm rather confused by how to manage edits to a pull request once it has been submitted; I might have broken other changes that Rob did to my pull request above, I have no idea how I would get such changes into my branch to merge them etc.

I wasn't really sure myself, so I found this:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

So the suggested changes you decide to commit in will be added to the branch you want to merge in with espruino/BangleApps:master, in this case hughbarney/BangleApps:master. Those will now be in your branch here on github, but not sync to your local copy if you cloned the repo to your computer. That should be resolved if you fetch or pull the changes from the remote copy on github (I think).

@bobrippling
Copy link
Collaborator

There's no problem in this case, what you've done is spot on imo - a merge (of upstream/master) and extra changes committed followed by a push is a good way to check. If git sees that anything might be overwritten, it'll fail at the push, so that can be used to check nothing's been missed :)

(in this case I only had comments, no changes to your branch, so nothing is lost)

@bobrippling
Copy link
Collaborator

Meant to say - I'm happy to merge if everyone else is?

@hughbarney
Copy link
Contributor Author

I wasn't really sure myself, so I found this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

Glad it is not just me. I'e going to try and do my next app all through the fork on github - and try and use the 'update fork' button which I could not find, maybe I was not signed into github.

@gfwilliams
Copy link
Member

Thanks! Looks good to me! Sorry about the confusion on the naming - I saw HRM Widget + and assumed the app ID was widhrm+ - thanks for making the change anyway, and I think that probably fits better with what we've done in other apps (run/runplus/etc)

@gfwilliams gfwilliams merged commit d57b30a into espruino:master Sep 22, 2023
1 check passed
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.

4 participants