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

Make profile top better in mobile mode #299

Merged
merged 13 commits into from
Feb 18, 2025

Conversation

seanmunson
Copy link
Contributor

@seanmunson seanmunson commented Feb 7, 2025

in mobile mode, "details" now becomes a pop-out that can be triggered by tapping the (i) button in the header

image

#296

Copy link

vercel bot commented Feb 7, 2025

@seanmunson is attempting to deploy a commit to the thieflord06's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Feb 7, 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 Feb 17, 2025 1:31pm

@thieflord06
Copy link
Member

Looks good.

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.

this feels like a very reasonable way to handle this, but I think the font size is too small, and the close button is also too small (visibly, and too small of a touch target). Beyond that I'm very much on board!

auto-merge was automatically disabled February 11, 2025 13:05

Head branch was pushed to by a user without write access

@thieflord06 thieflord06 requested a review from noahm February 11, 2025 19:34
@thieflord06
Copy link
Member

This looks and feels a lot better.

@noahm
Copy link
Collaborator

noahm commented Feb 11, 2025

Yup this looks a lot better. I need to do a bit more testing with the latest dev merged in, since this still has all the old double scrolling issues.

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.

Updated to the latest from dev just to check first, but the issue is still there: while the profile info panel is visible there's a large amount of extra horizontal scroll available for some reason. Nothing immediately stood out to me, so I'm not really sure why.

@thieflord06
Copy link
Member

It looks like it has something to do with the 'x' button.

@thieflord06
Copy link
Member

I just played around A LOT with trying to get the 'x' closer to the top of the container, with no luck. I guess it'll have to do as is.

@thieflord06 thieflord06 self-requested a review February 15, 2025 03:05
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.

For desktop we need to change back to the old way that handle history was being shown (In the bottom left corner). I think this was changed in #294

auto-merge was automatically disabled February 16, 2025 11:15

Head branch was pushed to by a user without write access

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.

Looks good.

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.

Updates to the extra profile look great. This also changes the way scrolling works again, to fix all the profile header and tabs to the top of the screen which I disagree with (at least in the form it is here, best to continue discussing in frontend or design channels)... but it seems that may have been an unstated goal of this PR that I was just unaware of? Not blocking this, but just wanted to call out the extra changes that don't seem described elsewhere.

@noahm noahm merged commit c7b9acc into ClearskyApp06:dev Feb 18, 2025
3 checks passed
@thieflord06 thieflord06 linked an issue Feb 24, 2025 that may be closed by this pull request
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.

Make profile description collapsible
3 participants