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

Unify disconnected pages #5391

Merged
merged 4 commits into from Feb 19, 2015

Conversation

@Flaburgan
Copy link
Member

commented Nov 11, 2014

After a discussion with @jhass about #5389 he pointed that it would be better to first unify every pages with the current design (white background, black text).

So here we are:
Registration page
registration

Connection page
connection

Password recovery 1
password1

Password recovery 2
password2

Old page was like that:
old-pwd

The "diaspora*" on the screenshots is the podname.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2014

Just a question, do we want the footer on the registration page too?

@jhass

This comment has been minimized.

Copy link
Member

commented Nov 11, 2014

Sure, why not? And I think English screenshots would be awesome! ;)

@svbergerem

This comment has been minimized.

Copy link
Member

commented Nov 11, 2014

@Flaburgan Have you already tried a responsive version of the sign up page? (.container-fluid instead of .container and drop the width: 80%) All other pages you modified are responsive.

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch from 84cf0d0 to 2ad504d Nov 12, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2014

@svbergerem I'd like to have bigger margins around the container, what would be the bootstrap way to do that if I don't manually fix the width to 80%? To use offsets?

@jhass footer added on registration page. I didn't change any text on those page, do you still need english screenshots?

@svbergerem

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

You can try this

.container-fluid
  .row-fluid
    .span10.offset1
      -# content

or this

.container-fluid
  .row-fluid
    .span8.offset2
      -# content
@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2014

okay so using offsets. I'll try.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 12, 2014

Done. But when the width is too small and the form goes under the text, it's not centered. Any tip on that?

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch from 2ad504d to 451d86e Nov 12, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2014

I keep getting "There was an error while loading data, please try again" on Travis, is anyone able to access the trace?

@goobertron

This comment has been minimized.

Copy link

commented Nov 13, 2014

Are you redesigning the sign-in / connection page further? It was recently redesigned by Steffen, your screenshot looks the same as his PR to me, and I think it works well so doesn't really need further redesign. Would be useful to know what (if any) changes you've made to that page.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2014

@goobertron only minor changes on the connection page. The big changes are on the forgot password and registration pages.

@jhass

This comment has been minimized.

Copy link
Member

commented Nov 13, 2014

Failing Scenarios:

cucumber features/desktop/change_password.feature:26 # Scenario: Attempt to reset password with invalid email

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch from 451d86e to 4b942e5 Nov 13, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2014

Okay, tests fixed. Should be ready to be reviewed.

@svbergerem if you have a suggestion to have content of the registration page centered on 360px width screen ;)

#new_user {
display: block;
margin: auto;
width: 400px;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Nov 13, 2014

Member

Please use max-width: 400px;, otherwise this will break on smaller screens.

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch from 4b942e5 to 8df7fdf Nov 13, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2014

@svbergerem done.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Nov 13, 2014

Alright, here comes a first, quick review:

  • To unify the pages you should consider dropping the new-btn and using standard Bootstrap buttons. We don't use those "new" buttons anywhere else so you could even drop them from app/assets/stylesheets/new_styles/_form.scss. Some might say that they look "too bootstrappy" but let's use a consistent button style which we can modify later.
  • Please add some margin between the TOS info and the continue button on the sign up page.
  • The form style is still different:
    sign up
    sign in
    • We have the big asterisk on all pages except for the sign up page.
    • The font for the pod name is different.
    • The borders are different. There is also some redundant border under the last input field on all changed pages except for the sign in page.
    • On the sign in page we use icons and the placeholder text describes the input field. On the sign up page we have description next to the input field. The width of the input field is different on different pages (forgot password 60%, reset password 55%). I guess that is based on the label text which is bad because the length of the label text depends on the language. That's why I used those icons.
  • The placeholder text on the forgot password page is wrong:
    wrong placeholder
  • Instead of centering the text of the sign up page on smaller devices I would consider completely dropping it so you immediately see the sign up form. You can do that by adding the class hidden-phone to the surrounding element, in this case to <h1 id="create-something-text"> (see http://getbootstrap.com/2.3.2/scaffolding.html#responsive)
  • I don't like the border color for invalid input fields. That's not part of your PR but it would be great if you could change it because it also affects the sign in page. If you don't include it in this PR I will open one for that issue. Here is how I would change the color:
diff --git a/app/assets/stylesheets/new_styles/_forms.scss b/app/assets/stylesheets/new_styles/_forms.scss
index 15aedde..382b658 100644
--- a/app/assets/stylesheets/new_styles/_forms.scss
+++ b/app/assets/stylesheets/new_styles/_forms.scss
@@ -92,12 +92,22 @@ textarea, input[type=text], input[type=password], input[type=search] {
   &:invalid:focus,
   &:invalid:required,
   &:invalid:required:focus {
-    border: 1px solid $border-dark-grey;
     outline: none;
     -webkit-box-shadow: none;
     box-shadow: none;
     color : $text-dark-grey;
   }
+
+  &:focus {
+    border: 1px solid $border-dark-grey;
+  }
+
+  &:invalid,
+  &:invalid:focus,
+  &:invalid:required,
+  &:invalid:required:focus {
+    border: 1px solid $border-grey;
+  }
@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2014

Okay I'll work on this. When I started the issue I thought about "unify" from a user point of view, but you're right, it makes even more sense from a code point of view too. So first step, let's rewrite the registration page from erb to haml.

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch 2 times, most recently from c3a8bd9 to 352a716 Nov 15, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2014

@svbergerem I'm sorry but to use placeholder instead of labels is really bad in term of accessibility. We can tweak that with CSS and JS, but just to remove the label is not an acceptable solution.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Nov 17, 2014

@Flaburgan

to use placeholder instead of labels is really bad in term of accessibility

That might be, I don't know much about accessibility on the web. All I am saying is that however we decide (labels or icons + placeholders + whatever is needed for accessibility) we should unify those 4 pages and if we choose labels then we should make sure that they work in all languages.

Thank you for the work you are doing. This is a really important improvement which needed to be done for a long time.

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2014

Looks like the file sessions.scss.css is used nowhere. am I right?

@Flaburgan Flaburgan changed the title Unify not connected pages [WIP] Unify not connected pages Dec 1, 2014

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch from 352a716 to c304468 Dec 1, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2014

Okay, I choose the option of an hidden label here only for accessibility and to use the placeholder to indicate to users the goal of the field. I wrote most of the css in a generic way so it can be used easily in every forms, the login.css and registration.css files are now really small. I added entypo icons.

This is still a work in progress, the captcha is not displayed correctly, I didn't fix the tests and have to check your points to be sure I didn't forget anything.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Dec 2, 2014

Looks like the file sessions.scss.css is used nowhere. am I right?

Nice catch, I guess you are right. grep -Rin "sessions" app/ lib/ vendor/ didn't return anything useful.

I know this is still WIP and I'll review this once you say you are ready but I saw that you changed $border-grey. We use it everywhere so I'd be really careful when I would change one of those colors. What was the reason? To use it as the border color for forms? Why is #ccc in this case better than #ddd? If this has to be #ccc I'd prefer adding a new color like $form-input-border-grey or anything like that. (And we should make that color consistent for the whole application but that might be out of scope for this PR. However if you change it, it would be great if you could open a new issue afterwards so we remember that we have to do it.)


label {
color : $text-dark-grey;
position: absolute;

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered color, left, position, top

This comment has been minimized.

Copy link
@ghost

ghost Feb 13, 2015

You talk too much @houndci, has someone already told you that ? Oh wait ! No ! You're a bot ! :p

&:invalid:required:focus {
box-shadow : none;
fieldset {
margin-bottom: 1em;

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered background-color, margin-bottom, position

width : 100%;
padding : 8px 0;
.entypo {
position: absolute;

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered color, font-size, left, position, text-align, top, width

box-shadow: none;
color : $text-dark-grey;
input[type=submit] {
text-align: center;

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Properties should be ordered display, padding, text-align, width

@@ -1,91 +1,115 @@
input,

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Avoid qualifying attribute selectors with an element.

outline: none;
box-shadow: none;
color : $text-dark-grey;
input[type=submit] {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Avoid qualifying attribute selectors with an element.


border : none;
input {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 13, 2015

Selector should have depth of applicability no greater than 2, but was 3

@svbergerem

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

I hope those comments are only there because this PR hasn't been rebased and so this commit is missing.

Sign in here: %{login_url}. If you’ve forgotten your sign-in details, you can ask for a reminder on that page.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

In most cases removing trailing whitespaces is a good idea. In this case all those lines belong to one email and I am not sure if those whitespaces are required. Please revert these changes unless you are sure that it doesn't break stuff.

This comment has been minimized.

Copy link
@Flaburgan

Flaburgan Feb 18, 2015

Author Member

There are other email texts in the file and this one is the only one with spaces here, I think it's safe to remove it.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 18, 2015

Member

Good point.

text-align: center;
display: block;
width: 100%;
padding: 8px 0;
}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

You can just remove those 6 lines defining the style of submit buttons and use Bootstrap classes for those buttons instead. I'll add a comment for each button so you see which classes are missing.

%br/
= link_to t('devise.shared.links.sign_in'), new_session_path(resource_name)
= hidden_field(:user, :remember_me, value: 1)
= f.submit t('devise.passwords.edit.change_password'), class: "btn"

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

the class should be btn btn-block

= link_to t('devise.shared.links.sign_in'), new_session_path(resource_name)
%i.entypo.mail
= f.text_field :email, class: "input-block-level form-control", required: true, autocapitalize: "off", placeholder: t('devise.passwords.new.email'), autocorrect: "off", autofocus: true
= f.submit t('devise.passwords.new.send_password_instructions'), class: "btn"

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

the class should be btn btn-block

- if AppConfig.settings.terms.enable?
%p#terms.text-center
= t('.terms', terms_link: link_to(t('.terms_link'), terms_path, target: "_blank")).html_safe
= f.submit t('.sign_up'), class: "btn", data: {disable_with: t('.submitting')}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

the class should be btn btn-large btn-block

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

I wrote in a previous comment that you should change the translation of registrations.new.sign_up. Instead you could also just use device.shared.links.sign_up.

This comment has been minimized.

Copy link
@Flaburgan

Flaburgan Feb 18, 2015

Author Member

Hm, is devise.shared.links.sign_up used elsewhere? Because it is translated by "Inscription" in French, which is a name. I'd like a verb here.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 18, 2015

Member

grep -Rin "devise.shared.links.sign_up" app/views/

If that is a problem for some languages you can of course just change registrations.new.sign_up.

margin: 12px;
font-size : 35px;
}

#sign-up-text {

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

looks like there is no #sign-up-text in the app anymore so you can just remove it

max-width : 95%;
#create-something-text,
#diaspora-hearts,
#sign-up-text {

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

you can remove #sign-up-text

new:
forgot_password: "Forgot your password?"
no_account: 'No account with this email exists'
reset_password: "Reset password"
email: "Email address"
email: "EMAIL ADDRESS"

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 14, 2015

Member

Why are those changes necessary? We already have text-tranform: uppercase for the placeholders.

This comment has been minimized.

Copy link
@Flaburgan

Flaburgan Feb 15, 2015

Author Member

Do you think that input:-moz-placeholder, input::-webkit-input-placeholder, input:-ms-input-placeholder are enough? Not sure it always works, but you're probably right, if some browsers don't display it uppercase, it's not a problem, this is only about design.

This comment has been minimized.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 15, 2015

Member

Add ::selection and ::placeholder selectors support.

They added support for the selector some time ago. :)

@svbergerem

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

I just finished my code review and there were some minor remarks. There are also some tests failing where you will have to change And I press "Continue" to And I press "Sign up" After resolving the remarks and fixing the tests this PR should be ready to merge.

@svbergerem svbergerem referenced this pull request Feb 15, 2015
2 of 2 tasks complete

@Flaburgan Flaburgan force-pushed the Flaburgan:unify-not-connected-pages branch from a452f1e to b3558d0 Feb 18, 2015

@svbergerem svbergerem added this to the next-major milestone Feb 18, 2015

@svbergerem svbergerem merged commit 2b19b4b into diaspora:develop Feb 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

svbergerem pushed a commit that referenced this pull request Feb 19, 2015

Steffen van Bergerem
@svbergerem

This comment has been minimized.

Copy link
Member

commented Feb 19, 2015

Thank you! :)

@Flaburgan

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2015

Finally! Sorry for the extra work here @svbergerem now I know where my mistakes came from I'll not do it again ;)

@Flaburgan Flaburgan deleted the Flaburgan:unify-not-connected-pages branch Feb 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.