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.

Sign up for GitHub

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

Jump to bottom

feat: Select preset for minimal page #2588

Merged
merged 16 commits into from
May 16, 2024
Merged

feat: Select preset for minimal page #2588

merged 16 commits into from
May 16, 2024

Conversation

hannahpurcell
Copy link
Collaborator

@hannahpurcell hannahpurcell requested a review from a team as a code owner May 9, 2024 22:06
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Coverage of commit 67aa5b0

Summary coverage rate:
  lines......: 93.7% (3232 of 3449 lines)
  functions..: 73.3% (1337 of 1824 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/skate_web/router.ex                                         |88.7%     62|16.2%   136|    -      0

Download coverage report

firestack
firestack reviewed May 10, 2024
View reviewed changes
lib/skate_web/router.ex Outdated Show resolved Hide resolved
firestack
firestack reviewed May 10, 2024
View reviewed changes
Copy link
Member

@firestack firestack left a comment

Choose a reason for hiding this comment

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

praise: loving this so far! Not able to finish review today but I've left a few comments.

@joshlarson
Copy link
Contributor

This makes a lot of sense to me! Consider me approving once Kayla approves.

I have some thoughts and questions about UX, but these are thoughts that should maybe be brought up to design, and make the most sense as follow-ups:

  • I still don't think that the minimal view should have any of the interactive components. I can add a follow-up task to get rid of them if you all agree.
  • Right now it's not particularly discoverable. Should there be a way to navigate to /minimal? Like a new nav link in the nav bar gated behind the same test group? I'm torn about this because it raises some thorny questions about users/roles/flags/etc (basically this would be perpetually available only to one user), but still.
  • Should there be some kind of error state in case the preset that you tried to refresh gets deleted? (Or in case you navigate to /minimal/oops or something like that)

@hannahpurcell
Copy link
Collaborator Author

hannahpurcell commented May 13, 2024 •
edited
Loading

@joshlarson agree 1 and 2 are questions for both design & product lead. # 3 I went ahead and added something rudimentary to redirect to /minimal, since that seems like a natural best guess and took 1 line change. While the other polish would be nice, I'd guess that once this "minimal page" is shipped, we have other detour priorities to work on before finessing this page

@github-actions GitHub Actions
Copy link

Coverage of commit b9b1be0

Summary coverage rate:
  lines......: 93.7% (3232 of 3449 lines)
  functions..: 73.3% (1337 of 1824 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/skate_web/router.ex                                         |88.7%     62|16.2%   136|    -      0

Download coverage report

firestack
firestack reviewed May 14, 2024
View reviewed changes
assets/src/components/minimalLadderPage.tsx Outdated Show resolved Hide resolved
assets/src/components/minimalLadder.tsx Outdated Show resolved Hide resolved
assets/src/components/minimalLadder.tsx Outdated Show resolved Hide resolved
assets/src/components/minimalLadder.tsx Outdated Show resolved Hide resolved
assets/css/app.scss Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Coverage of commit 8726767

Summary coverage rate:
  lines......: 93.5% (3240 of 3464 lines)
  functions..: 73.3% (1339 of 1827 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/skate_web/router.ex                                         |88.7%     62|16.2%   136|    -      0

Download coverage report

@github-actions GitHub Actions
Copy link

Coverage of commit 2489cad

Summary coverage rate:
  lines......: 93.6% (3241 of 3464 lines)
  functions..: 73.3% (1340 of 1827 functions)
  branches...: no data found

Files changed coverage rate:
                                                                  |Lines       |Functions  |Branches    
  Filename                                                        |Rate     Num|Rate    Num|Rate     Num
  ======================================================================================================
  lib/skate_web/router.ex                                         |88.7%     62|16.2%   136|    -      0

Download coverage report

@hannahpurcell hannahpurcell merged commit c8e1b21 into main May 16, 2024
15 checks passed
@hannahpurcell hannahpurcell deleted the hp/select-preset-for-minimal-page branch May 16, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshlarson joshlarson joshlarson left review comments

@firestack firestack firestack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hannahpurcell @joshlarson @firestack