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

UI/connectivity warning #1412

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

UI/connectivity warning #1412

wants to merge 11 commits into from

Conversation

jcovaa
Copy link
Contributor

@jcovaa jcovaa commented Dec 10, 2024

Closes #528
//

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@jcovaa
Copy link
Contributor Author

jcovaa commented Dec 10, 2024

Some prints of the warning.

@thePeras
Copy link
Member

thePeras commented Dec 11, 2024

I like the design. If the pharse was rephase to "Attention: No internet connection" would be better than demanding an action from the user. The user may be checking his next class, local data will be sufficient and no internet connection is required.

@jcovaa
Copy link
Contributor Author

jcovaa commented Dec 17, 2024

I liked more this design but it doesn't fit with some pages.
Edit: changed the phrase to "Attention: No internet connection" and "Atenção: Sem conexão à internet"

@jcovaa
Copy link
Contributor Author

jcovaa commented Dec 17, 2024

Some more design ideas

OR

Copy link
Member

@thePeras thePeras 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 a lot the option without background and red text on light and white on dark.

@R0drig0-P R0drig0-P requested a review from thePeras December 26, 2024 17:16
Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

One last request:
Don't upload an asset image to use as an icon. Import from the icon package Uni is using

@thePeras thePeras self-requested a review December 27, 2024 16:29
Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

Well done guys!

Don't forget to make the actions go green!

@@ -29,7 +30,10 @@ class AcademicPathPageViewState extends GeneralPageViewState {
@override
Widget getBody(BuildContext context) {
return ListView(
children: academicPathCards,
children: [
const ConnectivityWarning(),
Copy link
Member

Choose a reason for hiding this comment

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

To make the position of the ConnectivityWarning fixed, move it to the getTopNavBar method in GeneralPageViewState (however, note that the layout of the pages may change during the redesign).

Copy link
Contributor Author

@jcovaa jcovaa Dec 29, 2024

Choose a reason for hiding this comment

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

Following the conversation we had, I tried to change the _createTopWodgets method in the top_navigation_bar.dart class but there are 2 problems. The first is that the home page uses a getHeader method that creates the tittle differently from the other pages (the warning appears above instead of below the tittle). The second problem is that the size of the AppBar isnt dynamically changed when the warning is displayed, so it seems cut out. Do I have to increase its size or its my implementation that isnt correct?

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.

Put connectivity down warning on the screen
4 participants