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

Removing tables in favor of css #4

Closed
Lux-Delux opened this issue Mar 26, 2014 · 11 comments
Closed

Removing tables in favor of css #4

Lux-Delux opened this issue Mar 26, 2014 · 11 comments

Comments

@Lux-Delux
Copy link
Contributor

This is a lot of work, but I'm currently in the process of removing table elements from the new comment form, also the reply form later.

I'm a bit confused by the 'is mobile' parts at line 419 and 442 in javascript-mode.php.

When are these called and what is displayed?

@jacobwb
Copy link
Owner

jacobwb commented Mar 26, 2014

@Lux-Delux Keep in mind that a table was used for a reason, mainly that CSS tends to be horrible at keeping column sizes equal, especially when trying to take into account an uncertain number of columns (since any and all can be disabled), also where a CSS solution exists it's normally a method either introduced in CSS3 along with HTML5, or is at least only 2-3 years old. I worry about a non-table approach's compatibility with older web browsers.

"$is_mobile" is set in the "hashover.php" file at lines 216-220. It's set to "yes" if a visitor's user agent has "Android", "BlackBerry", or "phone" in it. It was implemented before tablets were popular, and thus its purpose is to add another table row and move the login button into the new row, making for a more compact design on small screens like smartphones.

@Lux-Delux
Copy link
Contributor Author

Ah thanks for the explanations...It seems I went ahead of myself a bit.

Ok, I'll keep it on the back-burner until I or someone else figure's out a safe yet modern approach.

Keeping the inputs out of table elements and in separate div's or similar would make them easier to theme - that was my idea, but not a big issue ;)

@jacobwb
Copy link
Owner

jacobwb commented Mar 26, 2014

@Lux-Delux If you feel confident that you can replace the table and don't mind testing it in some older browsers, you should certainly give it a try. I'm merely speaking from my own (many frustrating hours of) experience. You may well be better at CSS than I.

@Lux-Delux
Copy link
Contributor Author

Playing around in css... bleh, so much pain hehe... But I do think there are solutions to this.

First off we really should remove deprecated HTML code, for example align, and move everything possible to css.

A quick fix for the tables would be simply using the css table property, tried it, works and there are IE fixes available. However that's not the recommended approach (but we could use it as a temporary solution, just so the html elements have classes)

Having separate classes for each of the inputs without the table container drastically reduces any theming trouble later on.

I'll give my mock-up later

@Lux-Delux
Copy link
Contributor Author

Or we can simply use floats... do all the inputs need to be on the same row? For example, simply floating all elements to the left gives us something like this

untitled-1

Although in that case it would be ideal for the inputs to stretch (especially if there's less than all 4), unfortunately we'd need some javascript for that... Or make a simple php calculation based on the number of selected inputs that adds a width class which we then modify in css:

All 4 inputs : width: 22%
4

3 inputs : width: 29%
3

2 inputs: width:45%
2

We also adjust the login button width a bit with each. It's not perfect, but we got rid of html tables and can make things more responsive and sexy

@Lux-Delux
Copy link
Contributor Author

I've already rewritten the html part for this use case. If we go with this approach, the only thing that remains is making some sort of simple array to store the enabled inputs and count them.

I'd need help with this as I wouldn't want to slow down the php. Don't know where it would be best to store this array so we can use count or similar in javascript-mode.php, or if there's a more efficient way other than array please let me know!

Benefits:

  • cross-browser compatible
  • better/future friendly(we can always replace floats with a future alternative) html markup
  • no unnecessary if mobile php processing (the css will do it for us)
  • makes theming easy and approachable

Cons:

the width of the inputs won't be fully stretched to the edge of the container (could be achieved by some small javascript processing)

  • actually I managed to get around this with float:right and min-width. Works great!

@jacobwb
Copy link
Owner

jacobwb commented Mar 29, 2014

@Lux-Delux

I think "display: table;" and "display: table-cell;" is a good starting point. The table has to be removed nonetheless, once there isn't a table involved we can decide the best way to implement the columns.

With that said, I went ahead and replaced the tables with divs, and changed the CSS to style them appropriately. I have it looking 98% the same as it did with tables, the only issue is that the login button pushes the input boxes out of alignment on mobile. But I figure that's okay for now as that isn't the ideal nor the final place for the login button.

In regards to having PHP manage the column percentages, I think while that's possible, a common and familiar problem would still be present. Scaling. I've encountered so many problems with float-style column design, mainly that as the width gets longer the columns get more out of line or gain more padding to the opposite float side.

Notice that right-padding in your image examples; does that padding grow as you resize the containing element/window or when you zoom the page out? If so, that's what I'm talking about.

@Lux-Delux
Copy link
Contributor Author

Hmmm I don't understand what you exactly mean by the right padding... As I tested the float system it seems to work fine on all resoultions, I'll check what you did and test fresh.

Btw for adding classes to divs, I recommend at least 2 for the inputs, one is the input name and one general like hashover-input, so we can place table-cell or float or what not with less code in css

@Lux-Delux
Copy link
Contributor Author

Ok try this simple test and let me know if the problem still exists:

instead of display:table-cell use float:left on hashover-inputs
add width: 22%

That's for all inputs enabled. I'm resizing my Chrome browser window and no padding issues?

EDIT: by adding min-widths to all elements we also make sure they don't come out too small on low resoultions, and give the login button float:right and min-width:36px.

Example I copy pasted from inspector:

#hashover .hashover-inputs > * { float: left; width: 22%; min-width: 100px; } #hashover .hashover-login-input input { width: 36px; min-width: 36px; float: right; }

@jacobwb
Copy link
Owner

jacobwb commented Mar 30, 2014

@Lux-Delux

In my testing, I'm seeing padding issues, here's me demonstrating it:
https://drive.google.com/file/d/0BznAVozgsqIoTWpkaTJzQ2ZRUWs

Notice the space between the "Website" field and the login button grows and shrinks with the window. The point of a scalable design is too prevent this, and to have it look as close to identical across resolutions. In that regard, min-width is definitely not a good approach, as -- while it's true it prevents the inputs from getting too small -- the inputs break at different places depending on the screen resolution.

I think I'd rather use a CSS media screen max-width rule to change hashover-inputs from table-cells to blocks on resolutions smaller than, say, 640px.

Example:

@media only screen and (max-width: 640px) {
    #hashover .hashover-inputs > * {
        display: block;
    }
}

For now I think "display: table;" and "display: table-cell;" is the best approach, since it's widely supported and behaves correctly as well. However, in a year or two, it'll probably be safe to use the proper method of column-count, column-gap, column-width, etc.

@Lux-Delux
Copy link
Contributor Author

Ah I see, thanks for the demonstration, makes sense to keep what works for now ;) Important thing we moved from table markup in html

@jacobwb jacobwb added this to the 2.0 milestone May 5, 2014
@jacobwb jacobwb closed this as completed Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants