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

Use initial route instead of home widget #1190

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Conversation

bdmendes
Copy link
Collaborator

@bdmendes bdmendes commented Mar 6, 2024

Closes #1182
Closes #933

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

@bdmendes bdmendes force-pushed the fix/initial-route branch from e5774d3 to 76dec25 Compare March 6, 2024 15:04
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Merging #1190 (3dc431e) into master (33b3a0f) will increase coverage by 1%.
Report is 1 commits behind head on master.
The diff coverage is 20%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1190   +/-   ##
======================================
+ Coverage      16%     17%   +1%     
======================================
  Files         229     229           
  Lines        6957    6974   +17     
======================================
+ Hits         1086    1144   +58     
+ Misses       5871    5830   -41     

@limwa limwa changed the base branch from develop to master March 6, 2024 19:08
@limwa
Copy link
Member

limwa commented Mar 6, 2024

Good news! From my testing, this does fix the bug of the SessionProvider state being null. However, when I log out of the app, my credentials are still there. After I click to log in, the password should be cleared.

@limwa
Copy link
Member

limwa commented Mar 6, 2024

Note: When I log out and log back in, no lectures and exams show up, saying "Não existem aulas para apresentar".... I think this is something that we'll have to live for now...

@limwa
Copy link
Member

limwa commented Mar 6, 2024

Unfortunately, I am also my schedule is now showing errors (uni probably switched to the HTML fetcher) and there is no more "Open UC page in app" button in the lectures. It's also happening on the master branch, weird...

EDIT: fixed in #1192

uni/lib/view/navigation_service.dart Outdated Show resolved Hide resolved
@limwa limwa changed the base branch from master to develop March 6, 2024 21:31
@limwa
Copy link
Member

limwa commented Mar 6, 2024

Note: When I log out and log back in, no lectures and exams show up, saying "Não existem aulas para apresentar".... I think this is something that we'll have to live for now...

I managed to fix this issue by adding a notifyListeners call on the state provider notifier. Can you please give it a try to ensure this doesn't introduce any other bugs?

image

@LuisDuarte1 LuisDuarte1 changed the base branch from develop to master March 7, 2024 08:40
@LuisDuarte1 LuisDuarte1 changed the base branch from master to develop March 7, 2024 08:40
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

Please rebase the branch so that it can be merged into master

@bdmendes bdmendes force-pushed the fix/initial-route branch from 76dec25 to 8c925b3 Compare March 7, 2024 12:18
@bdmendes
Copy link
Collaborator Author

bdmendes commented Mar 7, 2024

@limwa I think setting the state null and removing the invalidate line as you suggested should be fine. I'll do it once I get to the PC

Copy link
Member

@limwa limwa left a comment

Choose a reason for hiding this comment

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

fixed formatting for you ;)

uni/lib/view/navigation_service.dart Outdated Show resolved Hide resolved
@bdmendes bdmendes force-pushed the fix/initial-route branch 2 times, most recently from 61663d9 to a829ebe Compare March 7, 2024 14:31
@limwa
Copy link
Member

limwa commented Mar 7, 2024

Please add screenshots of the new button if you can!

@bdmendes
Copy link
Collaborator Author

bdmendes commented Mar 7, 2024

@limwa seems to be working as expected. I also took the liberty of tackling #933 here:
Screenshot_20240307_142747

@bdmendes bdmendes force-pushed the fix/initial-route branch from 276ea5a to ece2f65 Compare March 7, 2024 14:43
@bdmendes bdmendes requested review from LuisDuarte1 and limwa March 7, 2024 14:44
@bdmendes bdmendes force-pushed the fix/initial-route branch from ece2f65 to d09e8b8 Compare March 7, 2024 14:46
@bdmendes bdmendes enabled auto-merge March 7, 2024 14:47
limwa
limwa previously approved these changes Mar 7, 2024
Copy link
Member

@limwa limwa 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! Thank you 🚀

@limwa limwa changed the base branch from develop to master March 7, 2024 14:56
@limwa limwa dismissed their stale review March 7, 2024 14:56

The base branch was changed.

@limwa limwa force-pushed the fix/initial-route branch from d09e8b8 to 3dc431e Compare March 7, 2024 14:58
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

🔨 🚀 🧏‍♂️

@bdmendes bdmendes merged commit 29b5663 into master Mar 7, 2024
6 checks passed
@bdmendes bdmendes deleted the fix/initial-route branch March 7, 2024 15:10
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.

Add 'Try Again' button to errored RequestDependantWidgetBuilder
3 participants