Update login and signup forms UI, and make search label visible #3362

adamjarling · 2018-10-31T15:59:28Z

Fixes #3361
This updates the following:

Log in and Sign up forms now have bootstrap styling and are improved UI.
Use the Hyrax logo in the masthead, per Bootstrap convention.
Fix the color of Search label in main search bar for different backgrounds depending where user is at in the app.
Consolidate the footer Copyright text to remove redundant "Copyright" text. Less text the better...

@samvera/hyrax-code-reviewers

houndci-bot · 2018-10-31T15:59:40Z

app/assets/stylesheets/hyrax/_login_signup.scss

+    }
+  }
+
+  input[type=submit] {


Avoid qualifying attribute selectors with an element.

Since the markup being styled is not under our control (coming from Devise gem), there's no other way I can wrap this element with a CSS selector class cleanly. Same as the next Hound complaint for type=email.

houndci-bot · 2018-10-31T15:59:40Z

app/assets/stylesheets/hyrax/_login_signup.scss

+
+  .field {
+    @extend .form-group;
+    input[type=email],


Avoid qualifying attribute selectors with an element.
Rule declaration should be preceded by an empty line

houndci-bot · 2018-10-31T15:59:41Z

app/assets/stylesheets/hyrax/_login_signup.scss

@@ -0,0 +1,19 @@
+// Login / Sign up styles needed because Devise gem controls the markup
+form.new_user {


Avoid qualifying class selectors with an element.
Selector new_user should be written in lowercase with hyphens

houndci-bot · 2018-10-31T15:59:41Z

app/assets/stylesheets/hyrax/_header.scss

  outline: 5px auto -webkit-focus-ring-color; // Chrome, Safari
  outline: -2px;
 }
+
+.navbar-brand {
+  padding: 12px 0 12px 15px;


Shorthands of length 4 are not allowed. Value was 12px 0 12px 15px

houndci-bot · 2018-10-31T16:06:25Z

app/assets/stylesheets/hyrax/_login_signup.scss

+    }
+  }
+
+  input[type='submit'] {


Avoid qualifying attribute selectors with an element.

houndci-bot · 2018-10-31T16:06:26Z

app/assets/stylesheets/hyrax/_login_signup.scss

+  .field {
+    @extend .form-group;
+
+    input[type='email'],


Avoid qualifying attribute selectors with an element.

houndci-bot · 2018-10-31T16:06:26Z

app/assets/stylesheets/hyrax/_header.scss

+
+.navbar-brand {
+  padding-top: 12px;
+  padding-right: 0px;


0px should be written without units as 0

houndci-bot · 2018-10-31T16:06:27Z

app/assets/stylesheets/hyrax/_header.scss

  outline: 5px auto -webkit-focus-ring-color; // Chrome, Safari
  outline: -2px;
 }
+
+.navbar-brand {
+  padding-top: 12px;


Properties should be ordered padding-bottom, padding-right, padding-top

jcoyne · 2018-10-31T22:06:17Z

I'm against adding a logo to the repository, as it's something that most institutions are going to want to provide for themselves and it increases the size of the repository. Can you host it elsewhere and just reference it?

Similarly, I'd prefer we don't include stylesheets/hyrax/_login_signup.scss as institutions that don't use devise (e.g. some sort of SSO) won't want that.

adamjarling · 2018-10-31T22:38:24Z

@jcoyne Ok, so for the logo, where else could or should it be hosted? Just thinking if that server goes down, or people are working on the app offline, it's a broken image vs the 15KB weight. I don't even know if it looks that great, just trying to do something to break this out of generic Bootstrap app look & feel.... but maybe that's not a concern?

Also, for the Sign In / Log In cleanup, I was thinking non-Devise users would not to be affected. For the life of me I can't figure out which HTML partial in Hyrax contains the Log In / Sign Up include code. Would a compromise be if someone could help me locate that file, we wrap the Devise code in a specific class selector, something like .external-auth-login-wrapper or whatever and target that way? It's the generic selector usage you're concerned with?

jcoyne · 2018-11-01T00:38:03Z

The devise code is in the devise gem. As for the image weight, if it’s this image, I wouldn’t worry about it being broken, if you care about controlling its availability you can move it into the hyrax app you generate.

adamjarling · 2018-11-01T13:33:37Z

The devise code is in the devise gem.

Would a better approach be to bring over the Devise partials? I tried this first but it didn't work...the Devise partial's path is app/views/devise/sessions/new.html.erb, which I created in Hyrax, re-started the Rails server, but it didn't serve. Looks like from the Devise docs you add a config/initializers/devise.rb file and include a line which seems to allow overrides (config.scoped_views = true), which I did, re-started the server, and the entire app chokes when that file is present.

Any idea what I'm missing?

After trying this route, that's when I created the stylesheets/hyrax/_login_signup.scss file to just extend standard Bootstrap form classes onto those elements.

Back to the point of this ticket, I'm thinking from the perspective of say a potential adopter of Hyrax, they go to Nurax to get an impression of the UI, and the Log In / Sign Up default views, the first thing they see, looks like it's neglected or unfinished. If it's no big deal to anyone else, I can just close this ticket...that's fine too.

jcoyne · 2018-11-01T14:30:50Z

@adamjarling I think the problem here is that you are trying to fix this in Hyrax, but the fix belongs in an application that uses Hyrax, such as Hyku or Nurax. Hyrax should just be a template, not a full solution, especially for something like authentication, which many institutions handle differently. This is why we don't require devise, we just use it for the default install. Adopters are encouraged to find another solution for auth, because devise is not suitable for all cases.

mbklein · 2018-11-01T16:45:16Z

IMO, if Hyrax is going to provide any kind of default login form out of the box, it should look good. So if this PR isn't worth merging, is the better course of action to remove the login form altogether?

jcoyne · 2018-11-01T18:12:07Z

@mbklein Hyrax doesn't provide a login form, devise does.

jcoyne · 2018-11-01T18:17:35Z

app/views/_logo.html.erb

@@ -1,4 +1,3 @@
 <a id="logo" class="navbar-brand" href="<%= hyrax.root_path %>" data-no-turbolink="true">
-  <span class="glyphicon glyphicon-globe" role="img" aria-label="<%= application_name %>" aria-hidden="true"></span>
-  <span class="institution_name"><%= application_name %></span>
+  <%= image_tag("hyrax_logo-bw.png", alt: application_name) %>


perhaps this could be done as something configurable such as

hyrax/app/views/layouts/homepage.html.erb

Line 3 in 5a9d1be

<div class="background-container" style="background-image: url('<%= banner_image %>')"></div>

I'm going to pull the logo out of this PR.

vantuyls · 2018-11-02T17:21:55Z

on the logo: i think it is a useful placeholder. is the primary issue here one of the size of the image?

vantuyls · 2018-11-08T00:13:25Z

i'm a little confused as tot he status of this work and conversation. @adamjarling thoughts?

adamjarling · 2018-11-08T14:04:58Z

@vantuyls The way I understand it, Devise is a gem which provides the markup for the Log In / Sign Up forms. This markup is not part of the Hyrax codebase. My changes are CSS additions (in the Hyrax codebase), which target Devise's Log In / Sign Up elements. My thought was if this is going to be visible on Nurax, then it should be cleaned up.

The suggestion was this styling code should be added elsewhere, .internal_test_app, instead of Hyrax?

So maybe the path forward is:

We do nothing and just let it look like it looks.
We add the styles to .internal_test_app? Then say someone or a new institution is evaluating this application on Nurax, one of the first thing they see (Sign Up), looks less neglected.

jcoyne · 2018-11-08T14:36:44Z

Some other alternatives:

Make this change in Nurax
Make a "bootstrap-devise" gem that provides a boostrap styled login for for devise and have hyrax generate that as a dependency into the host app (so it can be removed if necessary)

no-reply · 2018-11-12T17:29:05Z

I don't see any value in making this change in nurax, since it's not intended for actual use.

@jcoyne said:

@mbklein Hyrax doesn't provide a login form, devise does.

Since the hyrax install generators install devise, and the user code is deeply intertwined with it, I'm not sure what value this distinction has.

and:

Make a "bootstrap-devise" gem that provides a boostrap styled login for for devise and have hyrax generate that as a dependency into the host app (so it can be removed if necessary)

This seems like a lot of overhead to avoid putting some css/scss defaults in hyrax. I gather that you're arguing the changes here would make customization harder, but I'm not seeing it. Can you say more about what challenges you expect applications to face if we merge this?

jcoyne · 2018-11-12T18:25:59Z

@no-reply I'm just concerned that adding these styles to the gem make them "permanent", that is, hard to remove from your local install. How about moving these styles to the generator so that one could just rm the files if they aren't needed?

no-reply · 2018-11-12T19:48:32Z

I'm just concerned that adding these styles to the gem make them "permanent", that is, hard to remove from your local install.

Does it make them "permanent", or just hyrax managed/native? They can be overridden locally (right?), and we provide default styles in other places, even for engine provided views (e.g. blacklight catalog).

How about moving these styles to the generator so that one could just rm the files if they aren't needed?

The trade-off seems to be that any generated code needs to be manually updated to keep up with changes in the engine. The fragmentation of default styles between the engine and application seems like a risk to me.

We don't seem to generate .css or .scss files into applications in other cases. Is the thinking that this should be an exception because of the proximity to devise?

jcoyne · 2018-11-12T21:41:02Z

@no-reply We do generate https://github.com/samvera/hyrax/blob/master/lib/generators/hyrax/templates/hyrax.scss and this is intended to make it possible to keep things we don't want out of the compiled css. And, yes, i'm largely for keeping this out of the default hyrax bundle because it's so tightly coupled to Devise#database_authenticatable

no-reply · 2018-11-12T21:44:48Z

@jcoyne what if we moved the devise-specific styles into their own sheets and imported them separately, e.g.:

@import 'hyrax/devise';

That would meet your easily removable criteria without requiring a separate gem. Does this work for you?

Edit: indeed, it might be enough to remove import for 'hyrax/login_signup' from _hyrax.scss and add it back in templates/hyrax.scss.

no-reply · 2018-11-12T21:54:47Z

This needs to be rebased to pass CI, in any case, so I'm happy to make any changes needed if we can decide on a path forward.

jcoyne · 2018-11-12T22:08:13Z

I would feel better if hyrax/login_signup was imported by hyrax.scss (in the application) rather than by hyrax/hyrax.scss (in the engine)

… label readable

These syles impact views generated/managed by `devise` and the host application. We provide default styles to cover the most common cases. Rather than depending on these ourselves, we include the dependency in the `hyrax.scss` generated into the host application. This should help applications remove or override these styles more flexibly, while still allowing Hyrax to maintain the default styles internally.

no-reply · 2018-11-12T22:41:40Z

@jcoyne i pushed a new commit making that change, and rebased the rest. if it looks good, could you approve? I'll hold merge for your consideration.

Thanks for sticking this one out; i think the approach we landed on is a big improvement.

This takes advantage of the new login form styles in samvera/hyrax#3362.

adamjarling self-assigned this Oct 31, 2018

mjgiarlo added the in progress label Oct 31, 2018

houndci-bot reviewed Oct 31, 2018

View reviewed changes

adamjarling force-pushed the 3361-homepage-login-signup-cleanup branch from 05399af to 0378059 Compare October 31, 2018 16:06

houndci-bot reviewed Oct 31, 2018

View reviewed changes

adamjarling force-pushed the 3361-homepage-login-signup-cleanup branch from 0378059 to 1d5f6a7 Compare October 31, 2018 16:09

jcoyne reviewed Nov 1, 2018

View reviewed changes

adamjarling force-pushed the 3361-homepage-login-signup-cleanup branch from 1d5f6a7 to 19a0ab2 Compare November 2, 2018 16:18

adamjarling changed the title ~~Update login and signup forms UI, including Hyrax logo in top nav bar~~ Update login and signup forms UI, and make search label visible Nov 2, 2018

adamjarling and others added 2 commits November 12, 2018 14:39

Update login and signup forms styling in UI, and make homepage search…

a2e1646

… label readable

no-reply force-pushed the 3361-homepage-login-signup-cleanup branch from 19a0ab2 to f388fbe Compare November 12, 2018 22:39

jcoyne approved these changes Nov 12, 2018

View reviewed changes

no-reply merged commit c41b819 into master Nov 13, 2018

no-reply deleted the 3361-homepage-login-signup-cleanup branch November 13, 2018 03:55

no-reply removed the in progress label Nov 13, 2018

no-reply pushed a commit to no-reply/nurax that referenced this pull request Nov 13, 2018

Use new Hyrax devise styles

81ecf76

This takes advantage of the new login form styles in samvera/hyrax#3362.

no-reply mentioned this pull request Nov 13, 2018

Use new Hyrax devise styles samvera-labs/nurax-pre2023#311

Merged

bess pushed a commit to samvera-labs/nurax-pre2023 that referenced this pull request Nov 14, 2018

Use new Hyrax devise styles

663df81

This takes advantage of the new login form styles in samvera/hyrax#3362.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update login and signup forms UI, and make search label visible #3362

Update login and signup forms UI, and make search label visible #3362

adamjarling commented Oct 31, 2018

houndci-bot Oct 31, 2018

adamjarling Oct 31, 2018

houndci-bot Oct 31, 2018

houndci-bot Oct 31, 2018

houndci-bot Oct 31, 2018

houndci-bot Oct 31, 2018

houndci-bot Oct 31, 2018

houndci-bot Oct 31, 2018

houndci-bot Oct 31, 2018

jcoyne commented Oct 31, 2018

adamjarling commented Oct 31, 2018

jcoyne commented Nov 1, 2018

adamjarling commented Nov 1, 2018

jcoyne commented Nov 1, 2018

mbklein commented Nov 1, 2018

jcoyne commented Nov 1, 2018

jcoyne Nov 1, 2018

adamjarling Nov 2, 2018

vantuyls commented Nov 2, 2018

vantuyls commented Nov 8, 2018

adamjarling commented Nov 8, 2018

jcoyne commented Nov 8, 2018

no-reply commented Nov 12, 2018

jcoyne commented Nov 12, 2018

no-reply commented Nov 12, 2018

jcoyne commented Nov 12, 2018

no-reply commented Nov 12, 2018 •

edited

Loading

no-reply commented Nov 12, 2018

jcoyne commented Nov 12, 2018

no-reply commented Nov 12, 2018

		@@ -0,0 +1,19 @@
		// Login / Sign up styles needed because Devise gem controls the markup
		form.new_user {

Update login and signup forms UI, and make search label visible #3362

Update login and signup forms UI, and make search label visible #3362

Conversation

adamjarling commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne commented Oct 31, 2018

adamjarling commented Oct 31, 2018

jcoyne commented Nov 1, 2018

adamjarling commented Nov 1, 2018

jcoyne commented Nov 1, 2018

mbklein commented Nov 1, 2018

jcoyne commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vantuyls commented Nov 2, 2018

vantuyls commented Nov 8, 2018

adamjarling commented Nov 8, 2018

jcoyne commented Nov 8, 2018

no-reply commented Nov 12, 2018

jcoyne commented Nov 12, 2018

no-reply commented Nov 12, 2018

jcoyne commented Nov 12, 2018

no-reply commented Nov 12, 2018 • edited Loading

no-reply commented Nov 12, 2018

jcoyne commented Nov 12, 2018

no-reply commented Nov 12, 2018

no-reply commented Nov 12, 2018 •

edited

Loading