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

Split PirateCoinsPage into 2 Pages #244

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Split PirateCoinsPage into 2 Pages #244

wants to merge 3 commits into from

Conversation

PetalCat
Copy link
Contributor

@PetalCat PetalCat commented Nov 20, 2023

Description

This PR splits the user views for the Pirate Coins feature.

Closes: #192

Type of Change

  • ✨ New feature

@PetalCat PetalCat requested a review from a team as a code owner November 20, 2023 03:24
@lishaduck lishaduck added enhancement New feature or request dart Regards Dart code labels Nov 20, 2023
Copy link

netlify bot commented Nov 22, 2023

Deploy Preview for pattonville-wallet ready!

Name Link
🔨 Latest commit 31d93eb
🔍 Latest deploy log https://app.netlify.com/sites/pattonville-wallet/deploys/655d5018ca55f6000867ed45
😎 Deploy Preview https://deploy-preview-244--pattonville-wallet.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lishaduck lishaduck force-pushed the usersplit branch 2 times, most recently from ba7409e to 31d93eb Compare November 22, 2023 00:49
@lishaduck lishaduck changed the title feat: Split Pirate Coins page #192 Split PirateCoinsPage into 2 Pages Nov 22, 2023
Copy link
Member

@lishaduck lishaduck left a comment

Choose a reason for hiding this comment

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

# start of code generated by package:dart_define, DO NOT modify or remove these comments
dart_define.json
# end of code generated by package:dart_define, DO NOT modify or remove these comments

Copy link
Member

Choose a reason for hiding this comment

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

Why were these changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

nothing I did purposely

Copy link
Member

Choose a reason for hiding this comment

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

I'm removing it in #256 anyway, so it's not a big deal.

Comment on lines +46 to +59
page: PirateCoinsTeacherRoute.page,
path: "pirate-coins-teacher",
children: [
AutoRoute(
page: StatsRoute.page,
path: "stats",
title: (context, route) => "Stats",
),
],
title: (context, route) => "Pirate Coins",
),
AutoRoute(
page: PirateCoinsStudentRoute.page,
path: "pirate-coins-student",
Copy link
Member

Choose a reason for hiding this comment

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

It would be better for these to be nested and the difference in routes invisible, at least to students.

Copy link
Contributor

Choose a reason for hiding this comment

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

alr

@@ -35,7 +35,8 @@ List<AppletEntity> get _applets {
image: appletsFolder.pirateCoins.path,
color: const Color.fromARGB(255, 122, 194, 129),
// TODO(lishaduck): figure out how to use routes to keep this type-safe.
location: "/pirate-coins",
location:
"/pirate-coins-student", //not sure how to make this dynamic based on account type
Copy link
Member

Choose a reason for hiding this comment

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

Using route guards should remove to need to hardcode to student. And yes, I know it's not possible right now, which is why I'm working on #253.

Copy link
Member

@lishaduck lishaduck Dec 4, 2023

Choose a reason for hiding this comment

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

While you're at it, could you write some more tests to verify everything works here? I usually use Copilot to generate a basic test, and enhance it from there which makes it easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah was kinda planning to do that since the old ones were failing

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and it's the router tests failing, by the way. That's why, we should a) use go_router,1 b) nest the routes.

Footnotes

  1. I think it supports extracting the URL from a route, which is also needed for type-safe routing in the dashboard.

@lishaduck
Copy link
Member

Are you willing to take another go at this, @ParkerH27 / @PetalCat?

@PetalCat
Copy link
Contributor Author

PetalCat commented Jan 3, 2024

maybe... i gotta try and figure out what is going on

@lishaduck
Copy link
Member

@PetalCat? You still wanna finish this?

@lishaduck
Copy link
Member

I assume this can be closed, @ParkerH27?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart Regards Dart code enhancement New feature or request
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

Fix up the Teacher view
3 participants