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(kibbeh): add top padding to banned users #2759

Merged
merged 4 commits into from
May 15, 2021
Merged

fix(kibbeh): add top padding to banned users #2759

merged 4 commits into from
May 15, 2021

Conversation

pistasjis
Copy link
Contributor

This adds padding to banned users, because it looks awkwardly placed with the chat mode selector.
Before:
image
After:
image

Signed-off-by: Odyssey346 odyssey346@disroot.org

Signed-off-by: Odyssey346 <odyssey346@disroot.org>
@vercel
Copy link

vercel bot commented May 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

staging – ./kibbeh

🔍 Inspect: https://vercel.com/dogehouse-staging/staging/9GzVz9wRbEuPMwcEL9v88P6fhghD
✅ Preview: https://staging-git-fork-odyssey346-padding-dogehouse-staging.vercel.app

dogehouse – ./kibbeh

🔍 Inspect: https://vercel.com/benawad/dogehouse/7uYpBmL1FyxPWzZCXr7tAQgYqfZv
✅ Preview: https://dogehouse-git-fork-odyssey346-padding-benawad.vercel.app

storybook – ./kibbeh

🔍 Inspect: https://vercel.com/dogehouse-storybook/storybook/AbZvViQW3DQbrYEjAYKFpTARmHFW
✅ Preview: Canceled

[Deployment for c789921 canceled]

@akashrajum7
Copy link
Contributor

akashrajum7 commented May 14, 2021

Shouldn't the padding between each of the group be consistent, by a group I mean label and field?

Edit: Just noticed you used css styling to add padding, wouldn't it be better to stick to tailwind css for the sake of consistency?

@pistasjis
Copy link
Contributor Author

i've seen a couple of components do this way, i dont think theres gonna be multiple things using the exact same padding as this one

@pistasjis
Copy link
Contributor Author

also im not exactly sure what you meant with the padding thing

@akashrajum7
Copy link
Contributor

What I meant is, the space between each of the fields should all be same. Like the space between the first checkbox and chat cool down label, the bottom of the field and chat label and then again bottom of the field and Banned users label.

@pistasjis
Copy link
Contributor Author

Oh yeah, it's definitely inconsistent. I see what you mean.

Copy link
Contributor

@willdoescode willdoescode left a comment

Choose a reason for hiding this comment

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

make padding responsive instead of using pixel values.

Signed-off-by: Oxygemo <alej0hio2007@gmail.com>
@pistasjis
Copy link
Contributor Author

I didn't see that before le commit.... Crap.

@willdoescode
Copy link
Contributor

I didn't see that before le commit.... Crap.

Just change it to be responsive

@pistasjis
Copy link
Contributor Author

Ofcourse, but my mom is making me go to bed at 9pm in a fucking weekend. Can't do it tomorrow either apparently because I've been "not listening to her for the past week"

@pistasjis
Copy link
Contributor Author

So I can't at the moment. Will do tomorrow.

@willdoescode
Copy link
Contributor

So I can't at the moment. Will do tomorrow.

👍

@@ -122,7 +122,7 @@ export const BlockedFromRoomUsers: React.FC<BlockedFromRoomUsersProps> = ({}) =>

return (
<>
<div className={`flex mt-4 flex-col text-primary-100`}>
<div style={{ paddingTop: "15px"}} className={`flex mt-4 flex-col text-primary-100`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

use tailwind classes not custom styles

Signed-off-by: Oxygemo <alej0hio2007@gmail.com>
@pistasjis
Copy link
Contributor Author

aaaaaaaaaaaaaaaaaaaaaaaaaaaa

Signed-off-by: Oxygemo <alej0hio2007@gmail.com>
@pistasjis
Copy link
Contributor Author

I believe I've done both of your suggestions.

Copy link
Contributor

@willdoescode willdoescode 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 now

Copy link
Contributor

@willdoescode willdoescode left a comment

Choose a reason for hiding this comment

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

I just realized its still not responsive

@amitojsingh366 amitojsingh366 merged commit f0f08bc into benawad:staging May 15, 2021
@pistasjis pistasjis deleted the padding branch May 16, 2021 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kibbeh Related to the UI design implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants