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

move the no ext app tile to forward #2047

Merged

Conversation

zxkmm
Copy link
Contributor

@zxkmm zxkmm commented Mar 27, 2024

Move the ext app missing hint tile forward.

maybe can do same with flash err msg (but i won't do it).

Feel free to comment if you think this is not necessary. :)

@zxkmm zxkmm marked this pull request as ready for review March 27, 2024 15:39
@NotherNgineer
Copy link
Contributor

I like the added exclamation point to "Notice!" but I don't think the "External apps missing" text is going to reduce the number of confused users plus I don't like that it's hiding the title bar icons.

@zxkmm zxkmm changed the title stronger notice of missing ext app move the no ext app tile to formward Mar 28, 2024
@zxkmm zxkmm changed the title move the no ext app tile to formward move the no ext app tile to forward Mar 28, 2024
@htotoo
Copy link
Member

htotoo commented Mar 28, 2024

I like the latest edition of this pr

@NotherNgineer
Copy link
Contributor

It would help if you could add a screenshot to the PR or put test version on Discord. I would like to see what the updated screen looks like (I'll build it myself but it will take a bit longer)

@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 28, 2024

It would help if you could add a screenshot to the PR or put test version on Discord. I would like to see what the updated screen looks like (I'll build it myself but it will take a bit longer)

Ohhhh wait, no need to build, I'll upload the test bin now. Sorry

@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 28, 2024

test fw: https://discord.com/channels/719669764804444213/721387120421240872/1222912715635294229

I removed the icon of this tile, cuz i think this can make it more noticeable
(instead of making non-english speaker user saw/recongnize it as an app)
but feel free to against this. I'm not quite a visual designer so my point could be wrong

Sorry that i was just thinking this kind of small change ain't need tests

@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 28, 2024

20240328_222049

@NotherNgineer
Copy link
Contributor

I like that the "blacklist" is still supported, so I can hide this notice by putting "<Notice!>" in the blacklist file.

Is there any need for the less-than < and greater-than > symbols? IMO, I think Notice! without them would look better on the screen.

Also, I'm not sure that I like the placement of the Notice when the Back button is enabled on the menu. The Back button has always been in the upper left position when enabled, but it's shifted now:
SCR_0089

@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 28, 2024

Is there any need for the less-than < and greater-than > symbols? IMO, I think Notice! without them would look better on the screen.

I was planning to attract as much attention as possible, but if you think we don't need them, i can remove.

Also, I'm not sure that I like the placement of the Notice when the Back button is enabled on the menu. The Back button has always been in the upper left position when enabled, but it's shifted now

Thanks! I'll fix that now. I forgot that part.

@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 28, 2024

Back button fixed.

@NotherNgineer
Copy link
Contributor

Is there any need for the less-than < and greater-than > symbols? IMO, I think Notice! without them would look better on the screen.

I was planning to attract as much attention as possible, but if you think we don't need them, i can remove.

I don't know if we need to take a vote on this, but personally I think it looks better without the <> as in the screenshot below, so I would remove them:

SCR_0090

@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 29, 2024

No need to vote for this.
I removed it.

Copy link
Contributor

@NotherNgineer NotherNgineer left a comment

Choose a reason for hiding this comment

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

LGTM

@NotherNgineer NotherNgineer merged commit 90ff1e6 into portapack-mayhem:next Mar 29, 2024
3 checks passed
@zxkmm
Copy link
Contributor Author

zxkmm commented Mar 29, 2024

Thank you!

@zxkmm zxkmm deleted the strong_notice_of_ext_app_missing branch March 29, 2024 02:06
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