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

[Admin] Add new users admin items page #5874

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

MadelineCollier
Copy link
Contributor

@MadelineCollier MadelineCollier commented Oct 15, 2024

Summary

This PR is for issue: #5824

This migrates the users/:id/items page from the legacy soldius_backend to the new solidus_admin. Again there were no designs so I just tried my best to match to the existing components and layouts.

It also extracts the shared Last Login component and helper logic so that it can be reused across all the user pages without repetition.

Screenshots

State Old Admin New Admin
Purchases Screenshot 2024-10-14 at 6 38 22 PM Screenshot 2024-10-14 at 6 39 08 PM
No Purchases Screenshot 2024-10-14 at 6 38 32 PM Screenshot 2024-10-14 at 6 38 55 PM

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@kennyadsl
Copy link
Member

Thanks @MadelineCollier, each page is starting to look amazing!

One note, this button seems a bit off to me. Is this a pattern we have in the Figma file? Because we have the same action on the top right corner, and I feel like it's not necessary. WDYT?

Screenshot 2024-10-15 at 14 50 49@2x

@MadelineCollier
Copy link
Contributor Author

Thanks @MadelineCollier, each page is starting to look amazing!

One note, this button seems a bit off to me. Is this a pattern we have in the Figma file? Because we have the same action on the top right corner, and I feel like it's not necessary. WDYT?

Screenshot 2024-10-15 at 14 50 49@2x

Hey @kennyadsl I definitely agree that it's redundant, but it also mirrors the current (legacy) admin:

Screenshot 2024-10-14 at 6 38 32 PM

This was the flow for both the orders and the items page previously, so that's why I chose to implement it this way. But if you'd rather see something else there then I can definitely edit it, just let me know :)

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 98.52941% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.33%. Comparing base (c78a9de) to head (78e334d).

Files with missing lines Patch % Lines
.../components/solidus_admin/users/items/component.rb 97.91% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5874      +/-   ##
==========================================
+ Coverage   89.28%   89.33%   +0.04%     
==========================================
  Files         756      759       +3     
  Lines       17626    17678      +52     
==========================================
+ Hits        15737    15792      +55     
+ Misses       1889     1886       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MadelineCollier
Copy link
Contributor Author

As a sidenote, why does Codecov list this t() call as un-tested when it is exercised via the feature specs?
Screenshot 2024-10-15 at 3 35 23 PM

@tvdeyen
Copy link
Member

tvdeyen commented Oct 15, 2024

As a sidenote, why does Codecov list this t() call as un-tested when it is exercised via the feature specs?

Screenshot 2024-10-15 at 3 35 23 PM

my guess is because the user used in this test does not have a last_sign_in_at method?

@tvdeyen
Copy link
Member

tvdeyen commented Oct 15, 2024

I don't have the other screens on hand right now, but can we get rid of the "table in a box" layout and just display the table or use a more fitting look? I know there are no designs, but are there other examples of tables that could be used as blueprint?

@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Oct 16, 2024

@tvdeyen @kennyadsl I am eager to get this PR merged as I want to wrap the users admin (just the store credit page left, but that one is a bit more complicated than the others have been).

I have 3 outstanding pieces of feedback currently, none of which seem massive:

  1. The redundancy on the "Create Order" button.
  2. The Codecov flagging of t() (technically non blocking as its already approved by Codecov and within the desired coverage percentages).
  3. The request to take the table out of the box.

So for addressing each of these,
For:

  1. I have commented above (just let me know what the desired swap would be)
  2. I actually misread that Codecov flag, the t() was marked as previously un-covered, and if we merge this PR that un-covered line actually goes away (as it's moved from the orders component into the last_login_helper, which is 100% covered by the last_login_helper_spec.
  3. I tried taking the table out of the box, and in my opinion, it throws the page alignment off a bit, and makes the sidebar stick out as it's no longer on the same level as the table:

Before (table in box):
Screenshot 2024-10-16 at 3 50 34 PM

After (proposed removal of the box):
Screenshot 2024-10-16 at 3 51 13 PM

Since we don't have any designs, I am inclined to merge it as is (as the table in a box design persists across several admin pages currently). I think there is probably a much better way to present the info, but would prefer to fix/rework all the table in a box pages at once rather than holding up this PR while we think of an alternative. I am inclined to feel the same about feedback item 1, where it might not be the perfect solution, but it is the current standard in the new admin so until we think of a good swap we should leave as is.

What do you guys think? If you have clear desires for any of the above I am happy to tackle those points, but at the same time I am eager to move on from this page as there is a lot more admin work that needs to get done!

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

There are a couple of open points but for me, they are more general thoughts we should keep in mind and revisit once we have a clear answer, which will probably come by working on the other sections of the admin.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I am fine with the "table in a box". Thanks for trying to find a fix. I agree we need design for this.

Regarding the user stats component I have a suggestion and then I am eager to merge this in as well :)

This DRYs things up a bit so that all pages of the users admin can reuse
the same helper, translations, and component. Since the order index does
not use this stats component, but does make use of the last_login logic,
that was broken out into a separate helper for ease of use.
This migrates the `users/:id/items` page from the legacy
soldius_backend to the new solidus_admin.
These two components didn't require form_ids as they had no forms.
Purely informational display pages, only `get` requests, no forms in
sight.
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks

@MadelineCollier MadelineCollier merged commit fc2b820 into solidusio:main Oct 17, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants