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: fixed dropdown styling #4077

Merged

Conversation

ryandotfurrer
Copy link
Contributor

@ryandotfurrer ryandotfurrer commented Sep 2, 2024

I decreased the size of the modal as well as the dropdown in desktop. However, I increased the size on mobile to make it more inline with the mobile styling.

fix #4063

Description

This PR fixes bug #4063 by decreasing the size of the dropdown selector on desktop. I also took it upon myself to make the mobile styles more inline by increasing the size of the mobile dropdown. You can see these changes in the video below.

Related Tickets & Documents

Fixes #4063

Mobile & Desktop Screenshots/Recordings

Watch the video I made for this fix.

Desktop

Before
Arc Repositories  LetsGetTechnicalgridiron-survivor-000195

After
Arc Repositories  LetsGetTechnicalgridiron-survivor-000197

Mobile

Before
Arc Repositories  LetsGetTechnicalgridiron-survivor-000199

After
Arc Repositories  LetsGetTechnicalgridiron-survivor-000201

Steps to QA

Visit any repo of your liking and click the "Add to workspace" button. Please check it in mobile as well.

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

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

an-older-man-wearing-glasses-and-a-maroon-sweater-is-smiling

…own in add to workspace modal

I decreased the size of the modal as well as the dropdown in desktop. However, I increased the size
on mobile to make it more inline with the mobile styling.

fix open-sauced#4063
Copy link

netlify bot commented Sep 2, 2024

Deploy Preview for oss-insights ready!

Name Link
🔨 Latest commit 44986db
🔍 Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/66d831a9d1dd5d00085b74de
😎 Deploy Preview https://deploy-preview-4077--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 Sep 2, 2024

Deploy Preview for design-insights ready!

Name Link
🔨 Latest commit 44986db
🔍 Latest deploy log https://app.netlify.com/sites/design-insights/deploys/66d831a9e0016b0008fbbb1c
😎 Deploy Preview https://deploy-preview-4077--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.

.env Outdated Show resolved Hide resolved
remove line added in previous commit for NEXT_PUBLIC_EXP_API_URL
Copy link
Contributor

@zeucapua zeucapua left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Two things:

  1. Please attach screenshots for visual bug fixes like these; the PR template has a section!
  2. The mobile view uses a different component called AddToWorkspaceDrawer that requires a similar fix:
Screenshot 2024-09-03 at 07 41 53

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.

Going to take a peek at the fix, but just FYI the .env changes haven't been reverted yet. It still shows as a change in the PR.

CleanShot 2024-09-03 at 10 42 39

@ryandotfurrer
Copy link
Contributor Author

Thank you for the PR! Two things:

  1. Please attach screenshots for visual bug fixes like these; the PR template has a section!
  2. The mobile view uses a different component called AddToWorkspaceDrawer that requires a similar fix:

@zeucapua I am so sorry about that! I made a video but it wasn't showing up. I've fixed the PR comment to include the link to the video as well as appropriate screenshots.

The way I went about fixing this touched on the single-select component and AddToWorkspaceModal, I did not need to touch the AddToWorkspaceDrawer. Please let me know your thoughts on the fix and how you would have went about it.

@nickytonline
Copy link
Member

Just rerunning the E2E tests we have a bit of flakiness in CI at the moment. 😅

@nickytonline
Copy link
Member

Going to take a peek at the fix, but just FYI the .env changes haven't been reverted yet. It still shows as a change in the PR.

CleanShot 2024-09-03 at 10 42 39

@ryandotfurrer, if you run git checkout beta -- .env and commit those changes to your branch and push them, the .env file will be removed from your PR.

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.

Looks like I'm still seeing some issue on smaller screens. I dropped a suggestion for the fix.

components/atoms/Select/single-select.tsx Outdated Show resolved Hide resolved
@ryandotfurrer
Copy link
Contributor Author

Just rerunning the E2E tests we have a bit of flakiness in CI at the moment. 😅

We've experienced this in GIS as well, no worries 😅

@ryandotfurrer
Copy link
Contributor Author

Going to take a peek at the fix, but just FYI the .env changes haven't been reverted yet. It still shows as a change in the PR.
CleanShot 2024-09-03 at 10 42 39

@ryandotfurrer, if you run git checkout beta -- .env and commit those changes to your branch and push them, the .env file will be removed from your PR.

Done, ty for that! Any idea why the .env file was added in the first place? Just because I made changes to it? I made changes because Iw as trying to solve the /login issue before remembering it's an issue due to GIS.

@nickytonline
Copy link
Member

Going to take a peek at the fix, but just FYI the .env changes haven't been reverted yet. It still shows as a change in the PR.
CleanShot 2024-09-03 at 10 42 39

@ryandotfurrer, if you run git checkout beta -- .env and commit those changes to your branch and push them, the .env file will be removed from your PR.

Done, ty for that! Any idea why the .env file was added in the first place? Just because I made changes to it? I made changes because Iw as trying to solve the /login issue before remembering it's an issue due to GIS.

It not in the gitignore for our project as it's settings for people to be able to contribute to the project, so any changes made will appear if you git add ..

Maybe longer term we could have an example file and just have a step where we ask people to copy it. Pretty low on the priority list atm though. 😅

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.

Looks great in the drawer and in the modal now. Thanks @ryandotfurrer!

CleanShot 2024-09-03 at 15 51 19

CleanShot 2024-09-03 at 15 52 02

@ryandotfurrer
Copy link
Contributor Author

Looks great in the drawer and in the modal now. Thanks @ryandotfurrer!

Thanks for the collaboration, @nickytonline! I misunderstood the ticket at first but justify-between is an obvious solution 😅

I'll ask for clarification next time if there is any area for confusion on the ticket.

Copy link
Contributor

@zeucapua zeucapua left a comment

Choose a reason for hiding this comment

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

Great job! LGTM

@ryandotfurrer
Copy link
Contributor Author

Great job! LGTM

Thank you!

@nickytonline nickytonline changed the title fix(addtoworkspacemodal and single-select): reduced size of dropdown in add to workspace modal fix: fixed dropdown styling Sep 3, 2024
@nickytonline nickytonline added this pull request to the merge queue Sep 4, 2024
Merged via the queue into open-sauced:beta with commit 4fa8b7f Sep 4, 2024
11 checks passed
open-sauced bot pushed a commit that referenced this pull request Sep 10, 2024
## [2.62.0-beta.3](v2.62.0-beta.2...v2.62.0-beta.3) (2024-09-10)

### 🐛 Bug Fixes

* correct no insights message on contributor insights ([#4090](#4090)) ([49dfa03](49dfa03))
* fixed dropdown styling ([#4077](#4077)) ([4fa8b7f](4fa8b7f))
open-sauced bot pushed a commit that referenced this pull request Sep 10, 2024
## [2.62.0](v2.61.0...v2.62.0) (2024-09-10)

### 🍕 Features

* use `SplitButton` for repo page Add Workspace buttons ([#4062](#4062)) ([6cf13e6](6cf13e6))

### 🐛 Bug Fixes

* correct no insights message on contributor insights ([#4090](#4090)) ([49dfa03](49dfa03))
* fixed dropdown styling ([#4077](#4077)) ([4fa8b7f](4fa8b7f))
* use custom error component with Sentry error reporting for 500s ([#4078](#4078)) ([c3fd10e](c3fd10e))
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.

Bug: Workspace dropdown in Add to workspace modal it too big
4 participants