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 : Removing The Image from signup page section and making it responsive #43

Conversation

Rahilsiddique
Copy link
Contributor

Notes for Reviewers

This PR fixes #
#39

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Rahilsiddique <siddiquerahil19@gmail.com>
Signed-off-by: Rahilsiddique <siddiquerahil19@gmail.com>
@netlify
Copy link

netlify bot commented Dec 16, 2022

Deploy Preview for meshery-play ready!

Name Link
🔨 Latest commit d9531fb
🔍 Latest deploy log https://app.netlify.com/sites/meshery-play/deploys/63a29c929fbbff000982a44d
😎 Deploy Preview https://deploy-preview-43--meshery-play.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 settings.

Copy link
Member

@Nikhil-Ladha Nikhil-Ladha left a comment

Choose a reason for hiding this comment

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

Update the Discuss Callout CSS to use margin instead of padding. Also, increase the top margin by 2rem for wider views.
And, the callout is getting hidden in smaller device width. That shouldn't be happening. Instead, it should come up after the form and above the community callout.

site/src/components/SignupForm/signupform.style.js Outdated Show resolved Hide resolved
@Nikhil-Ladha
Copy link
Member

Also, please update the PR name to something more sensible. And, follow the same for future PRs.

@Rahilsiddique Rahilsiddique changed the title Feature/rahil siddique/signup page UI changes fix : Removing The Image from signup page section and making it responsive Dec 16, 2022
@Rahilsiddique
Copy link
Contributor Author

  • hey @Nikhil-Ladha , so all the changes requested by are done in the latest pr, padding is converted into margin at concerned places.
  • for DicussCallout, it was initially was in child div adjescent to signup form so when the website viewed on smaller devices it (DicussCallout) was comming before singup form.
  • To solve this issue, in my latest pr I have used the current width of the screen using using window.innerWidth and whenever the size is smaller than 1024px it will render the respective component at desired place using conditionaling rendering

this is my current implimentation if you have any thing to add orr want anykind of change on this please let me know and thankyou for your time.

Copy link
Member

@Nikhil-Ladha Nikhil-Ladha left a comment

Choose a reason for hiding this comment

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

I think we can handle this configuration using CSS itself, no need to use hooks here.
Just make all 3 divs under 1 parent div, and add a flex-wrap: wrap to the parent div, which would make the 3rd div (the Discuss Callout) come wrapped up and show at the bottom. Then, apply a position: relative and top: -(some magic number) and position it below the text up to 1024px or more. And, then after that update the CSS to top:0 for the Callout to make it move to the bottom.
Also, remove the console.log functions.

Signed-off-by: Rahilsiddique <siddiquerahil19@gmail.com>
Signed-off-by: Rahilsiddique <siddiquerahil19@gmail.com>
@Rahilsiddique
Copy link
Contributor Author

hey @Nikhil-Ladha, this is the final set of changes I have removed hooks and used position relative to adjust it in smaller devices and positioned it accordingly.
have a look and let me know what you think, Thanks !!

Signed-off-by: Rahilsiddique <siddiquerahil19@gmail.com>
Signed-off-by: Rahilsiddique <siddiquerahil19@gmail.com>
Copy link
Member

@Nikhil-Ladha Nikhil-Ladha 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!

@Nikhil-Ladha Nikhil-Ladha merged commit f6673ad into meshery:master Dec 21, 2022
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.

2 participants