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

fix: repos with long name partially stack #2534

Merged
merged 17 commits into from
Apr 5, 2024

Conversation

adiati98
Copy link
Member

@adiati98 adiati98 commented Jan 25, 2024

Description

This PR fixes the long repos' names that are partially stacked at the back of another name in the search input of the Explore tab.

The changes made here:

  • Add Tailwind className:

    • truncate to truncate overflowing text.
    • tracking-tighter to reduce letter spacing for better space.
    • inline-block to the <span> .
  • Remove Tailwind classNames:

Note: Changes made based on this conversation.

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Fixes #2335

Mobile & Desktop Screenshots/Recordings

Before

search before

After

final changes app

### After

search after

Steps to QA

  1. Go to https://opensauced.pizza/ and sign in.
  2. Click the "Explore" tab.
  3. Type, for example, "gpt" in the "Search Repository" input.
  4. You can see repos with long name are no longer stack on top of other repo's name.

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

finally gif

Copy link

netlify bot commented Jan 25, 2024

βœ… Deploy Preview for oss-insights ready!

Name Link
πŸ”¨ Latest commit 50ef5cf
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/660f64b942d65d0008e01d70
😎 Deploy Preview https://deploy-preview-2534--oss-insights.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.

Copy link

netlify bot commented Jan 25, 2024

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit 50ef5cf
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/660f64b9a2f0cd000859aea0
😎 Deploy Preview https://deploy-preview-2534--design-insights.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.

Copy link
Contributor

@takanome-dev takanome-dev left a comment

Choose a reason for hiding this comment

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

Wonder why there is no ellipsis after the text is truncated but another solution is to add inline-block in the span classes

SCR-20240126-itmr

@adiati98
Copy link
Member Author

adiati98 commented Jan 26, 2024

Wonder why there is no ellipsis after the text is truncated but another solution is to add inline-block in the span classes

SCR-20240126-itmr

This result is different from mine, locally. As you can see on the after screenshot in the PR form, I only get a single line for repo names, and the longer ones are cut. But without ellipsis.

Based on the docs, truncate gives ellipsis to truncate overflow text if needed.

I also wonder why px-4 here only gives padding left but not the right (again, locally on my end).

So, let me try your solution.

@adiati98
Copy link
Member Author

Wonder why there is no ellipsis after the text is truncated but another solution is to add inline-block in the span classes

SCR-20240126-itmr

@takanome-dev I follow your suggestion and made the changes as below:

  • Added inline-block className to the span.
  • Removed the truncate and tracking-tighter as they are no longer necessary.

I've tried several things and I found that only longer repo names are truncated and have ellipsis. But the longer owner names stay as is. That's why for longer owner names, ellipsis are not showing.

truncate

It will take me a while to figure this out if you want to have the suggestion in one line and ellipsis, and I don't want to hold this issue back. So, feel free to unassign me and give this to someone else. Thank you. 😊

cc: @nickytonline

@takanome-dev
Copy link
Contributor

Hey @adiati98, the changes looks good to me πŸ‘ Let's hear what @open-sauced/engineering think about it

Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

We definitely want each item in the list to be on one line and contain ... if it's too long. I'm wrapping up some feature work atm, but if you're still stuck on this @adiati98, I can take a peek to provide suggestions.

@adiati98
Copy link
Member Author

I'm honestly still stuck if we want to do ellipsis, @nickytonline .
Because ellipsis only applies to a longer repository name, but not the longer organization name.

When there's a longer organization name, they will stack on top of each other. And I haven't found a solution for this.

@adiati98
Copy link
Member Author

@nickytonline I've made the changes as per Bekah's suggestion to apply width in the <span>.

It didn't work for the longer organization name when I tried earlier (before my last commit) as I mentioned in this comment.

But after setting the width, it now works! The longer organization names are not in two lines as shown below.

Screenshot 2024-03-15 095905

However, as a note, for very long organization name, there's only ellipsis shown for the repo name as below.

Screenshot 2024-03-15 202258

components/atoms/Search/search.tsx Outdated Show resolved Hide resolved
cursor === index && "_cursorActive bg-slate-100",
"px-4 py-2 overflow-hidden break-all hover:bg-light-slate-2"
)}
className={clsx(cursor === index && "_cursorActive bg-slate-100", "px-4 py-2 hover:bg-light-slate-2")}
Copy link
Member

Choose a reason for hiding this comment

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

refactor (blocking): Instead of styling each span, configure it for all spans in it's parent.

Suggested change
className={clsx(cursor === index && "_cursorActive bg-slate-100", "px-4 py-2 hover:bg-light-slate-2")}
className={clsx(cursor === index && "_cursorActive bg-slate-100", "px-4 py-2 hover:bg-light-slate-2", "[&_span]:max-w-[12.3rem]")}

Is md:max-w-[11rem] required @adiati98? I'm only asking as the value is smaller for a larger screen.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nickytonline let me check it locally and get back to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nickytonline I've tested the suggestions locally, committed, and pushed them. We don't need the md:max-w-[11rem].
Below is the screenshot of the result. The blue box is longer repo name is truncated. And the green box is longer organization name, with the whole repo name gets truncated.

Screenshot 2024-04-04 201804

Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

This looks great @adiati98, could we make the 12.3 rem a little bit bigger, like 12.4 or 12.5 or does it get cut off (not ellipsis) at 12.4?

@adiati98
Copy link
Member Author

adiati98 commented Apr 4, 2024

@nickytonline making 12.3 rem to 12.4 rem or 12.5 rem do almost the same result, where the longer organization name gets the whole repo name truncated.

However, I tried two sizes:

  1. With 13 rem, and the result is better. The suggestion with longer repo name is shown longer before gets truncated (in the blue box) and the longer organization name can get at least a letter before gets truncated (green box).

    13

  2. With 13.4 rem. Longer organization name has two letters before gets truncated. However, bigger than this caused getting cut off.

    13-4

@nickytonline
Copy link
Member

@nickytonline making 12.3 rem to 12.4 rem or 12.5 rem do almost the same result, where the longer organization name gets the whole repo name truncated.

However, I tried two sizes:

  1. With 13 rem, and the result is better. The suggestion with longer repo name is shown longer before gets truncated (in the blue box) and the longer organization name can get at least a letter before gets truncated (green box).

    13

  2. With 13.4 rem. Longer organization name has two letters before gets truncated. However, bigger than this caused getting cut off.

    13-4

The 13rem one looks good. Thanks for your patience in this @adiati98!

@adiati98
Copy link
Member Author

adiati98 commented Apr 4, 2024

@nickytonline thank you also for your patience going back and forth with this! πŸ™

I'm not on my computer anymore. So, either I'll fix it tomorrow or if you can do it, Nick. 😊

@nickytonline
Copy link
Member

@nickytonline thank you also for your patience going back and forth with this! πŸ™

I'm not on my computer anymore. So, either I'll fix it tomorrow or if you can do it, Nick. 😊

No rush. Whenever you have a chance to push the changes.

@adiati98
Copy link
Member Author

adiati98 commented Apr 5, 2024

@nickytonline thank you also for your patience going back and forth with this! πŸ™

I'm not on my computer anymore. So, either I'll fix it tomorrow or if you can do it, Nick. 😊

No rush. Whenever you have a chance to push the changes.

Done 😊

@brandonroberts brandonroberts merged commit 1ed303b into open-sauced:beta Apr 5, 2024
11 checks passed
open-sauced bot pushed a commit that referenced this pull request Apr 5, 2024
## [2.16.2-beta.1](v2.16.1...v2.16.2-beta.1) (2024-04-05)

### πŸ› Bug Fixes

* now common OG image functions are in a utility file ([#3132](#3132)) ([450d874](450d874))
* repos with long name partially stack ([#2534](#2534)) ([1ed303b](1ed303b))
@adiati98 adiati98 deleted the fix/overflow-result-items branch April 5, 2024 19:37
open-sauced bot pushed a commit that referenced this pull request Apr 10, 2024
## [2.17.0](v2.16.1...v2.17.0) (2024-04-10)

### πŸ§‘β€πŸ’» Code Refactoring

* remove `/hub` and `/pages` subfolder routing ([#2963](#2963)) ([5c090f3](5c090f3))

### πŸ• Features

* add range selection to contributor profile page ([#3125](#3125)) ([641f9b2](641f9b2))
* add Skeleton to Repo Page ([#3124](#3124)) ([d661ffa](d661ffa))
* initial `/starsearch` layout ([#3148](#3148)) ([5c3d61b](5c3d61b))
* matched Contritbutor Insight edit page style to workspace UI ([#3109](#3109)) ([c9e3872](c9e3872))
* matched Repository Insight edit page style to workspace UI ([#3092](#3092)) ([c65b053](c65b053))
* repo charts style fixes ([#3151](#3151)) ([9b7197b](9b7197b))
* search for repositories ([#3140](#3140)) ([10ede95](10ede95))

### πŸ€– Build System

* update to Next.js 13.5.6 ([#3149](#3149)) ([b65b84c](b65b84c))

### πŸ› Bug Fixes

* now common OG image functions are in a utility file ([#3132](#3132)) ([450d874](450d874))
* pass selected repositories to new repository insight creation ([#3107](#3107)) ([3c57f94](3c57f94))
* repos with long name partially stack ([#2534](#2534)) ([1ed303b](1ed303b))
* swap text for stars/forks in repo page OG image ([#3145](#3145)) ([22fa602](22fa602))
* text area input does not focus when active and add highlight input is always focussed in post a highlight form ([#3008](#3008)) ([e9b9fed](e9b9fed))
* update API URL in contributor card edge function for pull requests ([#3157](#3157)) ([668a26b](668a26b))
* use OpenGraph image URL and remove server request from user profile ([#3137](#3137)) ([4e27f55](4e27f55)), closes [#3066](#3066)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants