Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

FIX : [BUG] No padding around 'Select a tab' label in mobile view #10150 #10191

Merged

Conversation

eugene4545
Copy link
Member

Closes #10150

Fixes Issue

Closes #10150

Changes proposed (

The select label in the UI was lacking proper padding, which led to alignment issues on smaller screens. To address this, I've added the following Tailwind CSS utility classes to the label's style: "px-6 py-1 mb-2" in the "select" tag inside "EventsTab.js" under the "Events" folder

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • [] My change requires changes to the documentation.
  • [] I have updated the documentation accordingly.
  • [] All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Before
Screenshot (1213)

After
Screenshot (1215)

Note to reviewers

@github-actions github-actions bot added the issue linked Pull Request has issue linked label Jan 28, 2024
Copy link
Member

@ThomasCode92 ThomasCode92 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!

@eugene4545
Copy link
Member Author

Hey there! Just checking in to see if there's anything else I can do to help get this PR merged. Let me know if there's any concerns about this

@@ -18,7 +18,7 @@ export function EventTabs({ tabs, eventType, onEventTypeChange }) {
label="Select a tab"
value={tabs.find((tab) => tab.key === eventType)?.title}
onChange={(e) => changeTab(e)}
className="block w-full rounded-md border-primary-medium-low dark:bg-primary-medium dark:focus:border-secondary-low dark:focus:ring-secondary-low focus:border-secondary-low focus:ring-secondary-low"
className="block w-full rounded-md border-primary-medium-low dark:bg-primary-medium dark:focus:border-secondary-low dark:focus:ring-secondary-low focus:border-secondary-low focus:ring-secondary-low px-3 py-1 mb-2"
Copy link
Member

Choose a reason for hiding this comment

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

Looks good! Perhaps consider adding the classes before the dark: and focus: variants. Doesn't Prettier usually take care of that automatically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the feedback, Arvind I've reordered the class names as suggested, with the dark and focus variants now at the end. As for Prettier, it doesn't seem to sort the class names automatically, so I manually rearranged them. Let me know if there's anything else I can do to improve the PR. Thanks again!

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @eugene4545!
Oh I didn't know about the extra plugin, thanks @ThomasCode92 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @ThomasCode92 , I have applied the tailwind prettier plugin on the code to rearrange the class name

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @arvind I was wondering if there are any remaining issues that need to be addressed.

Copy link
Member

@ArvindParekh ArvindParekh Feb 2, 2024

Choose a reason for hiding this comment

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

No, I think the pr solves the mentioned issue. Let's wait for others to review it and get it merged :)

Copy link
Contributor

@vrushabhgawas14 vrushabhgawas14 left a comment

Choose a reason for hiding this comment

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

Everything seems good.

@eugene4545
Copy link
Member Author

Hey! @kumarsonsoff3 I wanted to follow up on this PR that was opened about a month ago. Are there any outstanding concerns or feedback that I can help address to move this forward? Thank you for your time!

@eugene4545
Copy link
Member Author

Hey @kumarsonsoff3 , @ArvindParekh , @SaraJaoude! Hope everyone's doing well! I reached out to @kumarsonsoff3 about my PR a while back and wanted to follow up with the team. I understand you're all busy, and I appreciate your time and efforts. I'm looking for any feedback or updates to help move this along. If anyone has a chance to take a look, I'd really appreciate it! Thanks, everyone!

Copy link
Member

@kumarsonsoff3 kumarsonsoff3 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 to me. Great collaboration everyone! 👍

--
Sorry for delays, @eugene4545. I was away for a while and might missed your notification. Thanks for being patient, but please avoid tagging multiple maintainers at a time because every maintainer gets a notification for each activity on the project, it just creates extra noise for everyone

@kumarsonsoff3 kumarsonsoff3 merged commit c8e2bcf into EddieHubCommunity:main Mar 22, 2024
13 checks passed
@ArvindParekh
Copy link
Member

Thank you for your contribution @eugene4545 👍🏻

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] No padding around 'Select a tab' label in mobile view
5 participants