Skip to content

Conversation

@twolfson
Copy link
Contributor

@twolfson twolfson commented Nov 1, 2014

As reported in #7, Firefox dislikes having 2 inputs in 1 label. In this PR:

  • Relocated second input outside of label thus fixing behavior
  • Renamed label CSS to .input-row to make it more reusable and less conflict prone
  • Updated all containers for input's to use .input-row to preserve previous styling

This has not been tested with a live server. It has been tested with copy/paste/replace HTML/CSS in the browser. I attempted to get a vagrant box running but got frustrated with postgres setup.

https://github.com/twolfson/vagrant-nodebugme

On another note, I noticed a lot of bad practices in the CSS. Using content semantic classes can lead to unnecessary issues and extra work like having to update all label's to .input-row's. From my experience, using OOCSS conventions leads to less issues and more reusable classes. If you would like an explanation of how OOCSS could apply to this project, let me know and I will elaborate in another thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to rename this selector to .input-row__radio but I didn't want to overwhelm you with OOCSS and BEM conventions. This was the closest I could come to the original.

@chrisdickinson
Copy link
Contributor

Thanks for taking the time to diagnose and fix this bug!

This has not been tested with a live server. It has been tested with copy/paste/replace HTML/CSS in the browser. I attempted to get a vagrant box running but got frustrated with postgres setup.

I'm curious -- do you happen to recall what went wrong? It would be best if this project was easy to hack on, and if there's a roadblock you're hitting, I'd love to fix it. (Great work on the vagrant setup, by the way!)

If you would like an explanation of how OOCSS could apply to this project, let me know and I will elaborate in another thread.

If you have an idea of how you'd like to structure the CSS, feel free to open an issue that details what changes you'd like to make and we can discuss further there.

@chrisdickinson chrisdickinson merged commit 718dacf into nodebugme:master Nov 1, 2014
@twolfson
Copy link
Contributor Author

twolfson commented Nov 2, 2014

Awesome, thanks for the merge! Any idea when this fix will go out?

I am going to open up 2 separate issues for Vagrant/PostgreSQL and OOCSS. The OOCSS one might take a while (e.g. sometime later this week).

@chrisdickinson
Copy link
Contributor

The fix should be out sometime today. I want to make sure that I have some cache-busting in place before sending it out into the wider world :)

The OOCSS one might take a while (e.g. sometime later this week).

I'd like to have a discussion issue opened with your plans before changes are made -- ideally with what caused you pain and how you'd like to remedy it. In the issue, we can suss out any disagreements or potential pain points ahead of time, make some definite goals about where the CSS should head, and afterwards we can use that issue as a reference point for CSS style + CSS code review for newcomers to the project.

@twolfson
Copy link
Contributor Author

twolfson commented Nov 2, 2014

Yep, it will be a new issue (not a PR). The initial analysis/write up might take a while for me and I currently have my plate full with some other tasks. I will open an issue by the end of the next Sunday.

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