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

bug: dialog box is not responsive #2001

Closed
bdougie opened this issue Oct 25, 2023 · 19 comments · Fixed by #2043
Closed

bug: dialog box is not responsive #2001

bdougie opened this issue Oct 25, 2023 · 19 comments · Fixed by #2043
Assignees
Labels

Comments

@bdougie
Copy link
Member

bdougie commented Oct 25, 2023

image

@github-actions
Copy link

Thanks for the issue, our team will look into it as soon as possible! If you would like to work on this issue, please wait for us to decide if it's ready. The issue will be ready to work on once we remove the "needs triage" label.

To claim an issue that does not have the "needs triage" label, please leave a comment that says ".take". If you have any questions, please reach out to us on Discord or follow up on the issue itself.

For full info on how to contribute, please check out our contributors guide.

@nickytonline nickytonline added the core team work Work that the OpenSauced core team takes on label Oct 25, 2023
@nickytonline
Copy link
Member

This is related to discussion we had about adding a highlight previously @OgDev-01 @isabensusan.

For small screens, for now at least, it's probably best to have the modal take the full width.

@nickytonline
Copy link
Member

Related to this, on smaller screens, modals lose their top border radius. I fixed this in the account deletion flow I'm working on, but I'm going to create a good first issue for it as it's not directly related to my PR.

CleanShot 2023-10-25 at 08 57 39

@bdougie
Copy link
Member Author

bdougie commented Oct 27, 2023

Looks like highlights are also broken.

image

@bdougie bdougie changed the title bug: tagging a repo form is not responsive bug: dialog box is not responsive Oct 27, 2023
@github-actions
Copy link

Thanks for being interested in this issue. It looks like this ticket is already assigned to a contributor.

@bdougie bdougie assigned 5hraddha and unassigned OgDev-01 Oct 28, 2023
@bdougie bdougie added 🐛 bug Something isn't working and removed core team work Work that the OpenSauced core team takes on labels Oct 28, 2023
@bdougie
Copy link
Member Author

bdougie commented Oct 28, 2023

Assigned to you @5hraddha

@5hraddha
Copy link
Contributor

Assigned to you @5hraddha

Thanks @bdougie !

@5hraddha
Copy link
Contributor

Hi @bdougie,
After looking into the issue, I found that the highlight card (that opens on clicking a new highlight notification) is responsive for the most cases. The problem is the the structure of the PR or blog URL. On smaller screen, the css is set to add line breaks mid-word to the URL if needed reference. This works great when we've URLs like:

  1. https://github.com/netlify/next-runtime/pull/2287 - short URL
  2. https://dev.to/opensauced/the-need-for-recognition-in-open-source-247g - URL with many hyphens early on as the URL will break at hyphen whenever required.

But, when the URL is long and doesn't have hyphen early on in it, the URL doesn't break and wrap and overflows the card. Few examples of such URL are:

  1. https://github.com/TBD54566975/developer.tbd.website/pull/942 - Long URL without hyphen
  2. https://github.com/YurisCodingClub/accessibility-mentor/pull/58 - Long URL with hyphen but it comes quite later

In order to fix this issue, we can add css to add line breaks whenever necessary, without trying to preserve whole words reference.

This is how card looks without the fix:
image

This is how card looks after the suggested fix and URL broken without preserving whole words:
image

If the team is fine to break the URL like so, I'll raise a PR to fix the issue as suggested. Thanks!

@bdougie
Copy link
Member Author

bdougie commented Oct 31, 2023

@open-sauced/engineering please advise

@nickytonline
Copy link
Member

I think it's fine to break the URL so it fits/wraps but another option could be to use ellipsis. Twitter/X does this, e.g.

CleanShot 2023-10-30 at 22 59 53

The other thing I'd suggest on smaller screens is remove the margins (not padding) on the dialog so that it can get as much real estate as possible. I'll let @isabensusan chime in though.

@bdougie
Copy link
Member Author

bdougie commented Oct 31, 2023

I wouldn't scope this into this fix but we do have the option to swap to short urls in the future. Dub.co has an API we are not using and we have oss.fyi and pza.fyi options.

@nickytonline
Copy link
Member

I wouldn't scope this into this fix but we do have the option to swap to short urls in the future. Dub.co has an API we are not using and we have oss.fyi and pza.fyi options.

Yeah, this could be an enhancement in a future PR that we can discuss.

@bdougie
Copy link
Member Author

bdougie commented Oct 31, 2023

@5hraddha checking in to see if you had time to take a look.

@5hraddha
Copy link
Contributor

Yeah, @bdougie . I'm good with Nick's suggestion to keep the ellipses for longer urls and remove margins. I could start working on that and raise a PR. In the meanwhile, if there is any other suggestions, I could always add it. Thanks!

@nickytonline
Copy link
Member

nickytonline commented Oct 31, 2023

Sounds good @5hraddha. Feel free to tag me on your PR when it's ready.

@5hraddha
Copy link
Contributor

Sure, @nickytonline 👍 Thank you!

@5hraddha
Copy link
Contributor

5hraddha commented Nov 2, 2023

Hi @nickytonline,
I've created a PR to resolve the above issues as per your suggestions. Please, have a look whenever you've time. But, there are two things that I want to bring out to your notice:

  1. Right now, the process of creating or posting a highlight involves opening Add Repo popup from a popup. It'll be great to have an alternate UI pattern that doesn't involve opening nested popups.
  2. I also noticed that we need to revisit CSS for UI components, especially for the base components. There are many unnecessary CSS classes being used that is hindering the responsiveness. Just little cleanup will be great to maintain the UI.
    Please, let me know if there are any other stuffs you want me to tackle in this PR, I'll happy to add it. Thank you!

Copy link

open-sauced bot commented Nov 7, 2023

🎉 This issue has been resolved in version 1.74.1-beta.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

open-sauced bot commented Nov 9, 2023

🎉 This issue has been resolved in version 1.75.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@open-sauced open-sauced bot added the released label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants