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

107-add-starter-packs-packed #263

Merged
merged 23 commits into from
Jan 25, 2025

Conversation

seanmunson
Copy link
Contributor

@seanmunson seanmunson commented Jan 11, 2025

image

  • Needs more styling
  • fix URL
  • get bullet face of owner
  • get checks to pass
  • infinite scrolling / paging working
  • alternate non-icon in lieu of avatar working
  • pack originators with no avatars don't get a link to navigate to the list

Copy link

vercel bot commented Jan 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
clearsky-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2025 2:35pm

@thieflord06
Copy link
Member

This is coming together nicely.

Issues:

  • The hyperlink isn't pointing to the URL that is associated with each list, this is in the return
  • Conform formatting to other tabs
    Change the bullets to the profile pic of the list owner
    Add the divider line in-between each item
    Add dynamic sizing formatting to ensure description fits (reference Wrap list descriptions instead of elipses #264)

@thieflord06
Copy link
Member

Also, checks are still failing.

@thieflord06 thieflord06 linked an issue Jan 23, 2025 that may be closed by this pull request
@noahm
Copy link
Collaborator

noahm commented Jan 23, 2025

the one thing i noticed was that the packs created list total doesn't seem to agree with the actual data returned (says none created, but there's one there)

image

@thieflord06
Copy link
Member

thieflord06 commented Jan 23, 2025

That is a side effect of the on demand fetching on the backend that is happening.

Maybe a simple count on the return will do?

@thieflord06
Copy link
Member

Everything looks great! Last request, can you de-hyperlink if the profile doesn't resolve?

Copy link
Member

@thieflord06 thieflord06 left a comment

Choose a reason for hiding this comment

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

Passes functionality checks.

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

Functionally it all looks quite good! Code has just a few small details to polish up before merging

.gitignore Outdated Show resolved Hide resolved
src/detail-panels/packs/pack-view.jsx Outdated Show resolved Hide resolved
src/detail-panels/tab-selector.jsx Outdated Show resolved Hide resolved
src/common-components/account-short-entry.jsx Outdated Show resolved Hide resolved
src/types.d.ts Outdated Show resolved Hide resolved
@thieflord06
Copy link
Member

Changes complete, ready for re-review.

@thieflord06 thieflord06 self-requested a review January 25, 2025 16:13
Copy link
Member

@thieflord06 thieflord06 left a comment

Choose a reason for hiding this comment

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

Passes functionality checks.

Copy link
Collaborator

@noahm noahm left a comment

Choose a reason for hiding this comment

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

cleanup looks good!

@thieflord06 thieflord06 merged commit 02ce595 into ClearskyApp06:dev Jan 25, 2025
3 checks passed
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 starter pack view
3 participants