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

Refactor BLE Menu Options and Reposition "Press to Act" Display in WiFi Information #501

Closed
wants to merge 5 commits into from

Conversation

9dl
Copy link
Contributor

@9dl 9dl commented Nov 30, 2024

Proposed Changes

Streamlined the BLE menu options for better usability and adjusted the "Press to Act" display position to improve UI clarity and alignment.

Types of Changes

  • Refactor
  • UI/UX Enhancement

Verification

The changes can be verified by:

  1. Navigating to the BLE menu.
  2. Ensuring the options are streamlined and no longer redundant. (in code)
  3. Checking that the "Press to Act" text is correctly positioned as intended.

Testing

Manual testing was conducted to verify:

  • The BLE menu displays the updated options without like before and works.
  • The "Press to Act" text is in its new position across only on CYD.

Linked Issues

NONE

User-Facing Change

The BLE menu options are now more streamlined, and the "Press to Act" text has been repositioned for better readability (was bugged out for CYD).

Further Comments

Small refactor nothing big 😄

@9dl
Copy link
Contributor Author

9dl commented Nov 30, 2024

Before:
image
After:
image

@bmorcelli
Copy link
Collaborator

I fixed the message on screen here

tft.drawString("Press " + String(BTN_ALIAS) + " to act", 10, HEIGHT - 20);

BLE will be reworked, so I will leave it as it is for now, so I won't merge this PR

Thank you for your contribution

@bmorcelli bmorcelli closed this Dec 1, 2024
@9dl
Copy link
Contributor Author

9dl commented Dec 1, 2024

Hi @bmorcelli,

Thank you for your feedback! I see that you’ve chosen a simpler fix with the line:

tft.drawString("Press " + String(BTN_ALIAS) + " to act", 10, HEIGHT - 20);

While this works, I wanted to explain why I proposed using the #if !defined(HAS_TOUCH) approach in my fix. By incorporating the conditional preprocessor directive, my solution allows for device-specific customization, which ensures that the display behavior can be adapted based on the presence of a touch interface. This would be beneficial in cases where different devices may need different handling without modifying the core logic.

Additionally, I also refactored some of the BLE code, aiming for better maintainability and scalability. I’d love to hear your thoughts on this approach and whether you think it could be improved further.

If you believe the #if !defined(HAS_TOUCH) condition might not be suitable or if there are concerns I’m not considering, I’d really appreciate your insights.

Thanks again for your time and for reviewing my PR!

@bmorcelli
Copy link
Collaborator

Touch screen devices have HEIGHT set with HEIGHT=tft.getHeight() - 20; but is is set in the platformio.ini.. so we keep screen drawing equal to all devices.. so we try to no have behavior for different devices

I understand your idea for BLE code, and it is a good thing.. but it is to be reworked as soon as I finish the last touches in the Launcher.. I need to rework it to stop the restartings

@9dl
Copy link
Contributor Author

9dl commented Dec 1, 2024

Okay, thank you for the clarification! 😄

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.

2 participants