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

alignment of headings #3597

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Conversation

rohinish404
Copy link
Contributor

@rohinish404 rohinish404 commented Jan 17, 2023

Solves the issue #3596 i created

Fixes Issue

fix #3596

Changes proposed

Aligned the text properly. Attaaching a ss below to show proposed changes.

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.

Screenshot 2023-01-18 at 2 44 08 AM

Screenshots

Note to reviewers

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

It's great having you contribute to this project

Welcome to the community 🤓

If you would like to continue contributing to open source and would like to do it with an awesome inclusive community, you should join our Discord chat and our GitHub Organisation - we help and encourage each other to contribute to open source little and often 🤓 . Any questions let us know.

@github-actions github-actions bot added the small Pull request with less than 10 changed lines label Jan 17, 2023
Copy link
Member

@Aadarsh805 Aadarsh805 left a comment

Choose a reason for hiding this comment

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

removing the justify-between caused this on desktop [ignore, I ran the code in a branch with old code]

image

@rohinish404
Copy link
Contributor Author

But its looking perfectly fine when i open this in gitpod
Screenshot 2023-01-18 at 3 38 11 AM

@Aadarsh805
Copy link
Member

Aadarsh805 commented Jan 17, 2023

removing the justify-between caused this on desktop

image

Oh wait, i ran in wrong branch ig
totally mb, didn't even notice the wrong button

@Aadarsh805
Copy link
Member

@rohinish404 Hey Rohinish, could you try the suggested changes

@rohinish404
Copy link
Contributor Author

@Aadarsh805 which changes??

@Aadarsh805
Copy link
Member

@Aadarsh805 which changes??

The alignment error that is being caused by the margin bottom

@rohinish404
Copy link
Contributor Author

You are talking about the different margin bottom under the random profiles and the popular profiles right??
Screenshot 2023-01-20 at 12 22 29 PM

@Aadarsh805
Copy link
Member

You are talking about the different margin bottom under the random profiles and the popular profiles right?? Screenshot 2023-01-20 at 12 22 29 PM

No I'm talking about the mb-4 on H2 and not the icon, so because of one of them having the margin bottom they are not aligned

@rohinish404
Copy link
Contributor Author

Done..how it's looking now??

Aadarsh805
Aadarsh805 previously approved these changes Jan 20, 2023
Copy link
Member

@Aadarsh805 Aadarsh805 left a comment

Choose a reason for hiding this comment

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

All good now

@eddiejaoude
Copy link
Member

Please add a before and after screenshot

@rohinish404
Copy link
Contributor Author

Yeah sure!!
Here's how this was looking before-

213004999-73a9a30d-422d-4ff2-af80-5b8e14936dae.mov

And now it's looking-

Screen.Recording.2023-01-21.at.11.59.42.AM.mov

@Aadarsh805
Copy link
Member

add screenshot too, these video sometimes don't play, like rn one of them is not working on mine

@rohinish404
Copy link
Contributor Author

sure!!
Before-
Screenshot 2023-01-21 at 5 27 12 PM
After-
Screenshot 2023-01-21 at 5 16 28 PM

@Avi-88
Copy link
Member

Avi-88 commented Jan 23, 2023

Hi, just sharing my thought, wouldn't it look better if the headings are center aligned for smaller screens instead of how it looks currently ?

@rohinish404
Copy link
Contributor Author

@Avi-88 yeah but the headings below and above it are left aligned so it being cener aligned is disturbing the whole flow of the page.

@Avi-88
Copy link
Member

Avi-88 commented Jan 23, 2023

I was talking about making all the headings center aligned for smaller screens , what do you think about this ?

@rohinish404
Copy link
Contributor Author

Thats also a good idea..i was thinking the same before making this pr. @Aadarsh805 what do u think about this?

@Aadarsh805
Copy link
Member

Thats also a good idea..i was thinking the same before making this pr. @Aadarsh805 what do u think about this?

Yep it'd be better to have all center aligned actually

left aligned looks weird on mobile
image

@rohinish404
Copy link
Contributor Author

Okay, should i make the changes?

@Aadarsh805
Copy link
Member

Okay, should i make the changes?

Yeah go ahead

@eddiejaoude
Copy link
Member

Great collaboration everyone 👍 I think the improvements are a step in the right direction, the refresh icon doesn't need to go to it's own line when on mobile 👍

@Avi-88
Copy link
Member

Avi-88 commented Jan 23, 2023

Glad i could, help good luck with the PR @rohinish404 👍

@rohinish404
Copy link
Contributor Author

Great collaboration everyone 👍 I think the improvements are a step in the right direction, the refresh icon doesn't need to go to it's own line when on mobile 👍

Then i think we should go with all the headings being left aligned or if we want all the heading to be centered and the refresh icon to stay in the same line..it's looking like this👇
Which one will look better??
Screenshot 2023-01-23 at 6 28 34 PM

@Aadarsh805
Copy link
Member

Great collaboration everyone +1 I think the improvements are a step in the right direction, the refresh icon doesn't need to go to it's own line when on mobile +1

Then i think we should go with all the headings being left aligned or if we want all the heading to be centered and the refresh icon to stay in the same line..it's looking like thispoint_down Which one will look better?? Screenshot 2023-01-23 at 6 28 34 PM

Yeah this one's good

@rohinish404
Copy link
Contributor Author

Glad i could, help good luck with the PR @rohinish404 👍

Thank you for bringing a fresh perspective🙂

@rohinish404
Copy link
Contributor Author

Great collaboration everyone +1 I think the improvements are a step in the right direction, the refresh icon doesn't need to go to it's own line when on mobile +1

Then i think we should go with all the headings being left aligned or if we want all the heading to be centered and the refresh icon to stay in the same line..it's looking like thispoint_down Which one will look better?? Screenshot 2023-01-23 at 6 28 34 PM

Yeah this one's good

okay, i'll commit the changes.

@rohinish404
Copy link
Contributor Author

How it's looking now??

@Aadarsh805
Copy link
Member

How it's looking now??

Looks great to me

Copy link
Member

@eddiejaoude eddiejaoude 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 👍

@eddiejaoude eddiejaoude merged commit 55397c9 into EddieHubCommunity:main Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
small Pull request with less than 10 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heading not aligned properly on mobile on discover page.
4 participants