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

#5270 Align checkbox with the text. #5372

Merged
merged 4 commits into from
Apr 8, 2019
Merged

Conversation

shivam15
Copy link
Contributor

@shivam15 shivam15 commented Apr 5, 2019

Fixes #5270 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@welcome
Copy link

welcome bot commented Apr 5, 2019

Thanks for opening this pull request! This space is protected by our Code of Conduct - and we're here to help.
Dangerbot will test out your code and reply in a bit with some pointers and requests.
Also please refer here for installation help 💿
There may be some errors, but don't worry! We'll work through them with you! 👍🎉😄
It would be great if you can tell us your Twitter handle so we can thank you properly?

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 5, 2019

@publiclab/reviewers please review

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 5, 2019

screenshot

@plotsbot
Copy link
Collaborator

plotsbot commented Apr 5, 2019

2 Messages
📖 @shivam15 Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 Your pull request is on the master branch. Please make a separate feature branch) with a descriptive name like new-blog-design while making PRs in the future.

Generated by 🚫 Danger

@sashadev-sky
Copy link
Member

@shivam15 on my view of publiclab/post both locally and on the production site the checkbox is already aligned. Is this not the case for you? I couldn't find any recent PR's that would've fixed this.

Screen Shot 2019-04-05 at 1 49 36 AM

tagging @divyabaid16 because they opened the issue!

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 5, 2019

@sashadev-sky if you zoom the checkbox you can see checkbox and "Blur my location" is not in same line.
Screenshot 2019-04-05 at 12 23 50 PM

@divyabaid16
Copy link
Contributor

@sashadev-sky this issue was present in Mozilla Firefox.

@sashadev-sky
Copy link
Member

@divyabaid16 do you still see it there now?

The screenshot @shivam15 posted above is not the same alignment issue as the one you originally opened the issue for.

In Chrome or Firefox (v. 66.02) I also don't see either of these issues

What version of Firefox are you using? If it's browser specific I think it would make sense to apply a css rule for that specific browser that can be reusable rather than an inline style here

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 6, 2019

@sashadev-sky whats the conclusion ?

@divyabaid16
Copy link
Contributor

I will just check and let you know.

@divyabaid16
Copy link
Contributor

I'm still getting this in Chrome as well as Firefox
Selection_118

@sashadev-sky I am currently using Firefox version 67.0b2

@sashadev-sky
Copy link
Member

@divyabaid16 @shivam15 I looked into it a bit but it's hard to say why unless we all spent some time collaborating to try to figure it out.

I can confirm, though, that this PR doesn't have any negative impact on the UI for me so if it's improving alignment on other people's screens and @divyabaid16 confirms it improves hers I would say lets go forward with it.

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 6, 2019

@sashadev-sky @divyabaid16 may be once try with the changes from the pull request and see if everything looks fine from your end.

@sashadev-sky
Copy link
Member

@shivam15 Ok I will run your PR locally to double check!

@divyabaid16
Copy link
Contributor

divyabaid16 commented Apr 6, 2019

It's working for me @shivam15
You could have also used style = "float: left" 😄
Thanks!

@sashadev-sky
Copy link
Member

sashadev-sky commented Apr 6, 2019

With your changes:
chrome on the left firefox on the right

Chrome and firefox 1

Without your changes:
chrome on the left firefox on the right

c   f 2

This is my view.

@divyabaid16 can you please run his PR and post yours too maybe like I did a before and after (just the initial browser you were using - firefox - is fine)

also it wouldn't let me add you as a reviewer for this - your name wasn't showing up!

@sashadev-sky sashadev-sky self-requested a review April 6, 2019 14:04
Copy link
Member

@sashadev-sky sashadev-sky left a comment

Choose a reason for hiding this comment

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

Can we move this into an existing css file? There is an editor.css, maybe find an appropriate place to put it there? or map.css - nothing there yet but you can start it! I am trying to move this repo away from inline styling as its not good practice the way were using it - its all over the place

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 6, 2019

@divyabaid16 @sashadev-sky Done and Done!

@sashadev-sky

This comment has been minimized.

@sashadev-sky
Copy link
Member

I think the mis-alignment discrepancy is occurring because windows and mac (and probably other OS's) size these checkboxes differently, but Bootstrap positions them with pixel offsets assuming they're a specific size. See this issue on the Bootstrap page.

If you visit the bootstrap docs and toggle the w/h of the example checkboxes you can see this.

I don't think float is the right solution here because it's dependent on the Bootstrap styling already applied, which is error prone. But I found another solution, I am going to commit it here and if it doesn't work for you guys I'll revert it!

@sashadev-sky
Copy link
Member

@shivam15 @divyabaid16 Ok will you guys check if this fixes alignment for you?
@shivam15 you will need to do git pull origin master to pull in these changes, then set up Rails and run the server following the README instructions

@shivam15
Copy link
Contributor Author

shivam15 commented Apr 8, 2019

@sashadev-sky changes worked for me

Capture

Please merge the pull request if you are satisfied too.

@divyabaid16
Copy link
Contributor

I'm also getting the same result as @shivam15
It's working for me too @sashadev-sky
Thanks!

@sashadev-sky
Copy link
Member

@shivam15 @divyabaid16 woah how awesome guys! We just fixed a bootstrap bug they haven' fixed yet.

mentioning @jywarren to review because I wrote a lot of styling with !important declarations and this is also an important change to note for future usage of these particular bootstrap classes

@jywarren
Copy link
Member

jywarren commented Apr 8, 2019

This looks super. I love the inline comments explaining it. Thanks a lot everyone!!!!! 🎉

@jywarren jywarren merged commit 01c7ace into publiclab:master Apr 8, 2019
@welcome
Copy link

welcome bot commented Apr 8, 2019

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to PublicLab.org in the next few days, but first it will be published to https://stable.publiclab.org/ (it will take some minutes for this to load, and until then you may see logs from the build process). Please test out your work on this testing server and report back with a comment that all has gone well!
Do join our weekly check-in to share your this week goal and the awesome work you did 😃. Please find the link pinned in the issue section 📝
Now that you've completed this, you can help someone else take their first step!
Reach out to someone else working on theirs on Public Lab's code welcome page. Thanks!

Help others take their first step

Now that you've merged your first pull request, you're the perfect person to help someone else out with this challenging first step. 🙌

https://code.publiclab.org

Try looking at this list of `first-timers-only` issues, and see if someone else is waiting for feedback, or even stuck! 😕

People often get stuck at the same steps, so you might be able to help someone get unstuck, or help lead them to some documentation that'd help. Reach out and be encouraging and friendly! 😄 🎉

Read about how to help support another newcomer here, or find other ways to offer mutual support here.

icarito pushed a commit to icarito/plots2 that referenced this pull request Apr 9, 2019
* publiclab#5270 Align checkbox with the text.

* publiclab#5270 moving css to file

* Override Bootstrap bug
@sashadev-sky sashadev-sky added cross-browser CSS enhancement explains that the issue is to improve upon one of our existing features labels Apr 9, 2019
@grvsachdeva
Copy link
Member

grvsachdeva commented Apr 12, 2019

Awesome work everyone!

Suggestion for @shivam15 - It's good to open PR using a separate feature-branch, as it will help you in keeping your master branch clean.

Thanks!

jywarren pushed a commit that referenced this pull request Apr 19, 2019
* Remove unused css declaration

* Improved readability

* Try to remove error throw by extra space somewhere

* undo extra space in textarea

* Fix login button

* cleanup inline styling

* clean up login modal

* Forgot a div

* Fix activity checkbox

* Fix checkboxes in subscribe
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* publiclab#5270 Align checkbox with the text.

* publiclab#5270 moving css to file

* Override Bootstrap bug
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
…ab#5421)

* Remove unused css declaration

* Improved readability

* Try to remove error throw by extra space somewhere

* undo extra space in textarea

* Fix login button

* cleanup inline styling

* clean up login modal

* Forgot a div

* Fix activity checkbox

* Fix checkboxes in subscribe
digitaldina pushed a commit to digitaldina/plots2 that referenced this pull request May 12, 2019
…ab#5421)

* Remove unused css declaration

* Improved readability

* Try to remove error throw by extra space somewhere

* undo extra space in textarea

* Fix login button

* cleanup inline styling

* clean up login modal

* Forgot a div

* Fix activity checkbox

* Fix checkboxes in subscribe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-browser CSS enhancement explains that the issue is to improve upon one of our existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align checkbox with the text.
6 participants