Changed login form html structure to div elements #215

2019-02-16T11:13:22Z

laurensmartina commented

2019-02-16 11:13:22 +00:00

(Migrated from github.com)

The login form was structured in a table which is not easy to style.
In order to be more flexible with the way the login form can be styled
the html structure needed to be updated to div elements.

The login form was structured in a table which is not easy to style. In order to be more flexible with the way the login form can be styled the html structure needed to be updated to div elements.

giplt (Migrated from github.com) reviewed

2019-02-16 11:13:22 +00:00

dianawi (Migrated from github.com) reviewed

2019-02-16 11:13:22 +00:00

matthijskooijman (Migrated from github.com) requested changes

2019-02-18 19:13:50 +00:00

matthijskooijman (Migrated from github.com) left a comment

Looks like a good change. However:

I think this should also include some CSS changes to make the login form look the same (or at least ok) by default (it now changes a bit, in particular putting the login/cancel buttons vertically rather than horizontally).
I wonder if this changes makes sense in hypha.php, since that does not include the main CSS but instead has a bit of minimal inline CSS that now no longer applies. We could change this in hypha.php for consistency with the main code, but that would break consistency with the rest of hypha.php (that still uses tables).

Looks like a good change. However: - I think this should also include some CSS changes to make the login form look the same (or at least ok) by default (it now changes a bit, in particular putting the login/cancel buttons vertically rather than horizontally). - I wonder if this changes makes sense in `hypha.php`, since that does not include the main CSS but instead has a bit of minimal inline CSS that now no longer applies. We could change this in `hypha.php` for consistency with the main code, but that would break consistency with the rest of `hypha.php` (that still uses tables).

tammoterhark commented

2019-02-23 10:05:19 +00:00

(Migrated from github.com)

Not sure if this is within the scope of this PR, but ideaslly there should be no styling in the HTML. Currently there is in the HTML:

<div id="popup" style="left: 0px; top: 25px; visibility: visible;">

Please also take out the style statements.

Not sure if this is within the scope of this PR, but ideaslly there should be no styling in the HTML. Currently there is in the HTML: `<div id="popup" style="left: 0px; top: 25px; visibility: visible;">` Please also take out the style statements.

tammoterhark (Migrated from github.com) reviewed

2019-02-23 10:48:03 +00:00

tammoterhark (Migrated from github.com) left a comment

I've added some styling in the pull request.

matthijskooijman commented

2019-02-23 15:29:08 +00:00

(Migrated from github.com)

@tammoterhark, you seem to have moved button CSS to another place in the CSS file. Initially, I thought it was accidental, or at least unrelated to this change, but later I noticed that you're overriding the .button handling for the login form, so I suppose that the button CSS needs to be above the login form, hence you moved it. In this case, it would have been better to mention this in the commit message, or even put this in a separate commit, so it is easier to say something like "This only moves CSS, without any modifications".

However, I actually wonder if we should be reusing the same button class here. In other places, there is an input element with a button class, but here we're adding a div with button class around a button, which is actually a different kind of thing. So it's probably best to make sure that the same rules do not apply to both (either by doing input.button and div.button, or replacing the new button class with button_wrapper perhaps?). Though I wonder, do we really need that wrapper at all? You now use it to put the buttons side by side with inline-block and add some padding in between, but I'd think you could just make the input element inline (isn't that even the default) and use margin?

I also noticed some changes to the #popup CSS, but as we discussed this afternoon, I think we should move this into its own PR later.

@tammoterhark, you seem to have moved button CSS to another place in the CSS file. Initially, I thought it was accidental, or at least unrelated to this change, but later I noticed that you're overriding the .button handling for the login form, so I suppose that the button CSS needs to be above the login form, hence you moved it. In this case, it would have been better to mention this in the commit message, or even put this in a separate commit, so it is easier to say something like "This only moves CSS, without any modifications". However, I actually wonder if we should be reusing the same `button` class here. In other places, there is an `input` element with a `button` class, but here we're adding a `div` with `button` class *around* a button, which is actually a different kind of thing. So it's probably best to make sure that the same rules do not apply to both (either by doing `input.button` and `div.button`, or replacing the new `button` class with `button_wrapper` perhaps?). Though I wonder, do we really need that wrapper at all? You now use it to put the buttons side by side with `inline-block` and add some padding in between, but I'd think you could just make the input element `inline` (isn't that even the default) and use margin? I also noticed some changes to the `#popup` CSS, but as we discussed this afternoon, I think we should move this into its own PR later.

laurensmartina commented

2019-02-23 15:52:39 +00:00

(Migrated from github.com)

I wonder if this changes makes sense in hypha.php, since that does not include the main CSS but instead has a bit of minimal inline CSS that now no longer applies. We could change this in hypha.php for consistency with the main code, but that would break consistency with the rest of hypha.php (that still uses tables).

Removed changes in hypha.php.

> * I wonder if this changes makes sense in `hypha.php`, since that does not include the main CSS but instead has a bit of minimal inline CSS that now no longer applies. We could change this in `hypha.php` for consistency with the main code, but that would break consistency with the rest of `hypha.php` (that still uses tables). Removed changes in `hypha.php`.

matthijskooijman (Migrated from github.com) approved these changes

2020-01-04 20:40:16 +00:00

matthijskooijman (Migrated from github.com) left a comment

Looks good right now. Just one minor comment about indentation, if that's fixed this looks good to merge.

data/themes/default/hypha.css

					
				@ -331,0 +343,4 @@

				 background: #ddd;

				 color: #000000;

				}

There are mixed spaces/tabs here.

Also, all this new CSS uses tabs or 3 spaces for indentation, where the rest of the file uses single spaces.

laurensmartina (Migrated from github.com) reviewed

2020-01-06 18:17:29 +00:00

data/themes/default/hypha.css

					
				@ -331,0 +343,4 @@

				 background: #ddd;

				 color: #000000;

				}

fixed

Sign in to join this conversation.

No reviewers

dianawi

giplt

matthijskooijman

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!215

Rows
Columns