New layout for topiclist lists #1331

cyberalien · 2013-04-10T20:09:24Z

New layout for .topiclist lists

Adjustments for .html files include:

Wrapping content of dt in div.list-inner
Adding extra tab for contents of div.list-inner
Adding class name to ul.topiclist to indicate how many columns are there

RFC: http://area51.phpbb.com/phpBB/viewtopic.php?f=108&t=44139
Ticket: http://tracker.phpbb.com/browse/PHPBB3-11489

nickvergessen · 2013-04-10T20:39:05Z

Can you provide screenshots of new vs old?

cyberalien · 2013-04-10T21:32:15Z

@nickvergessen

Normal width: http://www.trushkin.biz/temp/Screen%20Shot%202013-04-11%20at%2000.29.44%20.png
Wide browser window: http://www.trushkin.biz/temp/Screen%20Shot%202013-04-11%20at%2000.29.52%20.png
Small browser window: http://www.trushkin.biz/temp/Screen%20Shot%202013-04-11%20at%2000.29.57%20.png

Lists look exactly the same, the only difference is scaling.

melvingb · 2013-04-10T21:44:59Z

I like this implementation,
With this the forumlist does not deformed to see it at different resolutions or am I wrong?.

cyberalien · 2013-04-10T21:48:00Z

Yes, it does not get deformed because with this solution combined width of all columns is always 100% of container's width.

nickvergessen · 2013-04-10T21:56:41Z

phpBB/includes/ucp/ucp_notifications.php

+		$template->assign_vars(array(
+			strtoupper($block) . '_COLS' => count($notification_methods),
+			strtoupper($block) . '_COLS1' => count($notification_methods) + 1,
+			strtoupper($block) . '_COLS2' => count($notification_methods) + 2


, at the end of this line,
also use sizeof() once and put it in a variable?

cyberalien · 2013-04-10T22:10:26Z

@nickvergessen Fixed

cyberalien · 2013-04-10T22:24:58Z

Forgot about RTL stuff. Also removed class name related to this PR that isn't used.

p · 2013-04-11T03:04:43Z

tests/functional/notification_test.php

@@ -43,7 +43,7 @@ public function test_user_subscriptions($checkbox_name, $expected_status)
 		$crawler = $this->request('GET', 'ucp.php?i=ucp_notifications&mode=notification_options');
 		$this->assert_response_success();

-		$cplist = $crawler->filter('.cplist');
+		$cplist = $crawler->filter('.table1');


Going from a meaningful to a meaningless class name?

Going from list to a table. Nether old nor new class name is specific to that page. There was one .cplist list, now there is one .table1 table.

p · 2013-05-04T08:09:49Z

This goes into 3.2.

cyberalien · 2013-05-04T08:20:06Z

It shouldn't. This PR is basically a bug fix for 2 issues:

Topic lists using mix of pixels and percentages, which ruins layout at low resolution
Notification settings layout that uses topiclist incorrectly

Do you want to release 3.1 with bugs?

naderman · 2013-05-04T19:53:35Z

Why hasn't either of those issues been reported as a bug?

naderman · 2013-05-04T19:55:10Z

Can't you fix the notification settings layout separately - that would make this PR smaller and each of them easier to review and understand all the implications?

cyberalien · 2013-05-04T20:25:11Z

Notification settings bug was discovered only when I was working on this PR. It is related to issue this PR solves, so I thought it would be ok to fix them both. I'll create a ticket and move it to separate PR.

As for topic lists layout, while it is technically a bug, it was done intentionally during 3.0 development because of limitations of very old browsers (IE6 and below) that were actual back then. So I'm not sure if it should be treated as bug or improvement.

p · 2013-05-06T16:43:05Z

phpBB/styles/prosilver/template/ucp_notifications.html

-					<li class="row<!-- IF notification_types.S_ROW_COUNT is odd --> bg1<!-- ELSE --> bg2<!-- ENDIF -->">
-						<dl>
-							<dt>
+			<table class="table1" cellspacing="1">


Why is cellspacing being used in 2013?

cellspacing is actually HTML5 invalid too. (of course, the adm/style files are littered with table cellspacing and border attributes, so somebody may wanna look at removing that one day too)

It could be changed, but that would require changing all tables. This code uses exactly same table code as other tables in prosilver.

p · 2013-05-06T16:44:50Z

If there is a bug being fixed here, there should be aticket of type "bug" filed, with an appropriate bug description.

"Currently forum columns use mix of percentages and pixels for width and margin/padding, causing columns to stretch at different resolutions and at low resolution break layout." - what resolutions? was this working before? if yes in which version? etc.

cyberalien · 2013-05-11T20:41:18Z

Marked as WIP because notification settings fixes were moved to separate pull request and should be removed from this PR.

cyberalien · 2013-05-12T16:48:09Z

Removed notification settings changes because they are now in separate PR and unmarked as WIP.

PHPBB3-11489

cyberalien · 2013-05-14T06:20:56Z

Squashed commits and rebased on latest develop, so notification settings changes would be included from develop.

EXreaction · 2013-05-20T00:21:44Z

phpBB/styles/prosilver/theme/content.css

+
+dl.icon dt, dl.icon dd {
+	min-height: 40px;
+	*min-height: 32px;


This should go in tweaks.css (along with the similar hacks)?

Yes. I'll move it there

PHPBB3-11489

EXreaction · 2013-05-20T14:46:22Z

phpBB/styles/prosilver/theme/tweaks.css

@@ -29,4 +29,21 @@ dl.details dd {
 /* Headerbar height fix for IE7 */
 #site-description p {
 	*margin-bottom: 1.0em;
-}


Please re-add newline at end of file

You are reading it wrong. There was no newline character at the end of tweaks.css, I've added new line and then code after it.

Vjacheslav is correct.

EXreaction · 2013-05-20T14:57:51Z

Everything else looks good to me, I can merge this after that's fixed.

iMattPro · 2013-05-24T04:18:54Z

phpBB/styles/prosilver/theme/content.css

+}
+
+dl.icon dt, dl.icon dd {
+	min-height: 40px;


@cyberalien I think this is problematic... It just looks weird now, the topic rows are all a bit taller than usual and the content is misaligned vertically in them towards the top as a result, and topic icons are now positioned to the bottom left... Removing this restores the original height and topic-icon locations to the way they have always been.

With the 40px min height (not attractive):

Without the 40px min-height (better and as it was):

Good point. It could be decreased, but it is needed to prevent row from being smaller than icon height.

How about 35px ? Looks good at 35 IMO.

That sounds reasonable.

Were you gonna adjust it, or were you expecting me to ??

I can do it or you can submit patch, doesn't matter

I'll leave it to you if you probably still have this branch active in your repos :)

http://tracker.phpbb.com/browse/PHPBB3-11577

I went ahead and made a #1451

nickvergessen reviewed Apr 10, 2013
View reviewed changes

p reviewed Apr 11, 2013
View reviewed changes

p reviewed May 6, 2013
View reviewed changes

cyberalien mentioned this pull request May 11, 2013

Fix notification settings layout #1400

Merged

cyberalien added 2 commits May 14, 2013 09:17

[ticket/11489] CSS changes for topiclist lists

3352131

PHPBB3-11489

[ticket/11489] Template changes for new topiclist layout

0d8ba50

PHPBB3-11489

EXreaction reviewed May 20, 2013
View reviewed changes

[ticket/11489] IE7 adjustments

d09c666

PHPBB3-11489

EXreaction reviewed May 20, 2013
View reviewed changes

EXreaction merged commit d09c666 into phpbb:develop May 20, 2013

iMattPro reviewed May 24, 2013
View reviewed changes

Uh oh!

New layout for topiclist lists #1331

New layout for topiclist lists #1331

Uh oh!

Conversation

cyberalien commented Apr 10, 2013

Uh oh!

nickvergessen commented Apr 10, 2013

Uh oh!

cyberalien commented Apr 10, 2013

Uh oh!

melvingb commented Apr 10, 2013

Uh oh!

cyberalien commented Apr 10, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyberalien commented Apr 10, 2013

Uh oh!

cyberalien commented Apr 10, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p commented May 4, 2013

Uh oh!

cyberalien commented May 4, 2013

Uh oh!

naderman commented May 4, 2013

Uh oh!

naderman commented May 4, 2013

Uh oh!

cyberalien commented May 4, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p commented May 6, 2013

Uh oh!

cyberalien commented May 11, 2013

Uh oh!

cyberalien commented May 12, 2013

Uh oh!

cyberalien commented May 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EXreaction commented May 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment