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

Alarm type and settings; fix drag keyboards colors settings #3001

Merged
merged 10 commits into from
Sep 19, 2023

Conversation

lauzonhomeschool
Copy link
Contributor

Screen real estate is important, it impacts how fast we can use and see important stuff on the Bangle.
I added settings to show or not certain Alarm features (e.g. I never use "Confirm", I just click the watch button to save); the default value for those settings match the current behaviour (i.e. no change to existing users unless they select an other config).
I also prefer to show overflow text rather than to have to see "..." and have to click once more to see the full text, so I added a setting to allow this. (OFF by default)
I also added one more optional (not shown by default) text input called "Group" (in a future PR, when the setting to show the Group is ON, Alarms/Events where the user typed the same "Group" string will appear under a sub-menu of the same name as the Group (e.g. "B-DAY").

I fixed the colors settings in the drag keyboards (not sure if this is a new behaviour or if I just happened to have the fixed version on my watch already and didn't notice).

@thyttan
Copy link
Collaborator

thyttan commented Sep 11, 2023

I added settings to show or not certain Alarm features (e.g. I never use "Confirm", I just click the watch button to save); the default value for those settings match the current behaviour (i.e. no change to existing users unless they select an other config).

I think this can make sense - thanks!

I also prefer to show overflow text rather than to have to see "..." and have to click once more to see the full text, so I added a setting to allow this. (OFF by default)

Did you notice that too long strings will draw outside of their menu entry? That's mainly why I implemented the "..." cut off. Is this handled when overflow is on? Or is that actually what you're after? Should the menu entry be able to change height to accommodate longer strings?

I also added one more optional (not shown by default) text input called "Group" (in a future PR, when the setting to show the Group is ON, Alarms/Events where the user typed the same "Group" string will appear under a sub-menu of the same name as the Group (e.g. "B-DAY").

If the functionality isn't available yet I'd think it's better to not add this code now but later when it's done instead.

I fixed the colors settings in the drag keyboards (not sure if this is a new behaviour or if I just happened to have the fixed version on my watch already and didn't notice).

Please can you break these out in separate commits at least? Or even PRs. It makes it easier to undo the app specific changes if later we notice something broke. (If you want to I can do a PR to your fork/branch to do this)

Thanks!

@lauzonhomeschool
Copy link
Contributor Author

For the overflow, yes, long strings will draw outside of their menu entry (and still generally be readable), and that's what I'm after. (maybe others won't find it "pretty enough", that's why this feature is behind a config)
For the Group, even though the "displaying by group" is not available, being able to already group event types is still handy (and behind a config, so it won't bug anybody who don't care for grouping).

Yes, if you could please do a PR to my fork/branch and break it however you like, I'll just reopen PR(s) for the structure you send.

@thyttan
Copy link
Collaborator

thyttan commented Sep 12, 2023

I just made a PR to lauzonhomeschool:alarm_type_and_settings, so if you pull that in it will update on this PR 👍

As for the rest I think that's fair enough - ping @gfwilliams.

I haven't gone through the checklist yet.

Split up "alarm settings (incl group), drag keyboard settings fix" in app specific commits
@thyttan
Copy link
Collaborator

thyttan commented Sep 18, 2023

Just pinging you again @gfwilliams

@gfwilliams
Copy link
Member

Ahh - I though I was waiting for you to go through the checklist...

Please could you define the default values at the top when you load the settings, like:

const settings = Object.assign({
  showConfirm : true,
  showAutoSnooze : true,
  showHidden : true
}, require('Storage').readJSON('alarm.json',1)||{});

And then you wouldn't need these ==null ? checks every time you used the var and I think it'd really tidy it up - same for the settings page as well.

My worry is that in future if someone were to try and change the default value, they'd have to do it all over the code and it wouldn't be very obvious

Also the CI check threw up a warning about data : [ name:"alarm.json" ] not being in the metadata, so please could you add that too?

@thyttan
Copy link
Collaborator

thyttan commented Sep 18, 2023

Ahh - I though I was waiting for you to go through the checklist...

Sorry about that!

@lauzonhomeschool
Copy link
Contributor Author

Done. Thanks for the advices.

@gfwilliams
Copy link
Member

That's great, thanks! Just made some minor tweaks, and merging now

@gfwilliams gfwilliams merged commit eab1c32 into espruino:master Sep 19, 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.

3 participants