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

WIP: New Alarm Layout #246

Merged
merged 5 commits into from
Dec 15, 2023
Merged

Conversation

SuhasDissa
Copy link
Member

@SuhasDissa SuhasDissa commented Dec 13, 2023

Moved all the clutter from alarm row into a separate alarm settings sheet

  • Need more testing ( in both 24 hour and 12 hour formats)

closes #239

@SuhasDissa SuhasDissa requested review from M00NJ and Bnyro December 13, 2023 09:23
Copy link
Member

@Bnyro Bnyro left a comment

Choose a reason for hiding this comment

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

Looks great, only some minor things I noticed when testing:

  1. The "Sound" switch can't be disabled for some reason, it always stays enabled.
  2. The "Sound" row has a divider between the sound selection menu and the switch, while the "Vibrate" row doesn't. I propose to add the divider to the "Vibrate" row too.
  3. The "Label" text field could use the fill width in my opinion - currently there's some free space at the right.
  4. The area that can be used for changing the time in the number picker is quite small, I'd suggest to add some horizontal padding to it that also reacts to scroll gestures.

This should probably also solve the performance issue when scrolling? :)

@SuhasDissa
Copy link
Member Author

  1. Currently there's no way to disable alarm sound. (I'm planning on adding it)
  2. In material3 the divider is used to show that the option is clickable ( check the attached screenshot for example), We could add a divider to the vibrate row if we add more vibration patterns
  3. 4.This PR is a WIP. I'll fix it eventually

image

Copy link
Member

@M00NJ M00NJ left a comment

Choose a reason for hiding this comment

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

I like many of these changes, like hiding the labels when empty and the reordering of the alarm prefs. I don't really like moving it to a separate sheet though. I think having all information on one screen is an advantage and the less navigation is required the better. I also like leaving the alarms expanded to see when they will ring at a glance, which wouldn't be possible anymore and instead require navigating back and forth. AOSP and G-Clock also work with animated visibility, which is the best choice for this use case imo. Performance issues are mostly a problem with compose and will be improved over time. Also I haven't noticed any myself and my device is a four years old mid ranger, so I doubt they're a major inconvenience. I'd be for taking most of these changes and applying them to the alarm cards. I think that would look really nice.

@SuhasDissa SuhasDissa changed the title New Alarm Layout WIP: New Alarm Layout Dec 14, 2023
@SuhasDissa
Copy link
Member Author

SuhasDissa commented Dec 15, 2023

I also like leaving the alarms expanded to see when they will ring at a glance, which wouldn't be possible anymore and instead require navigating back and forth.

Does this fix the problem?

@SuhasDissa SuhasDissa merged commit f864638 into you-apps:main Dec 15, 2023
1 check passed
@SuhasDissa SuhasDissa deleted the new-alarm-layout branch December 17, 2023 12:43
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.

Alarm labels are changed to other alarm lables when deleting one
3 participants