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

Improve navbar #2310

Merged
merged 1 commit into from
Jun 24, 2015
Merged

Improve navbar #2310

merged 1 commit into from
Jun 24, 2015

Conversation

tagliala
Copy link
Contributor

Moves gravatar near user email
Removes alt attribute from gravatar (useless md5 encoded email)
Also adds support for collapsed navbar

Before

screen shot 2015-05-27 at 18 20 49

After

screen shot 2015-05-27 at 18 56 25

screen shot 2015-05-27 at 18 56 44

@tagliala
Copy link
Contributor Author

$ CI_ORM=active_record CI_DB_ADAPTER=sqlite3 rspec ./spec/integration/rails_admin_spec.rb:134
Run options:
  include {:locations=>{"./spec/integration/rails_admin_spec.rb"=>[134]}}
  exclude {:mongoid=>true, :skip_active_record=>true}

Randomized with seed 3788
.

Top 1 slowest examples (0.74006 seconds, 99.9% of total time):
  RailsAdmin secondary navigation does not cause error when email is nil
    0.74006 seconds ./spec/integration/rails_admin_spec.rb:134

Finished in 0.74083 seconds (files took 2.86 seconds to load)
1 example, 0 failures

Randomized with seed 3788

😕

@tagliala
Copy link
Contributor Author

Devise 3.5.1 is causing the failure

@tagliala
Copy link
Contributor Author

What a lucky man, master branch also fails 🍀

~/dev/rails_admin on master $ CI_ORM=active_record CI_DB_ADAPTER=sqlite3 rspec ./spec/integration/rails_admin_spec.rb:134
Run options:
  include {:locations=>{"./spec/integration/rails_admin_spec.rb"=>[134]}}
  exclude {:mongoid=>true, :skip_active_record=>true}

Randomized with seed 53656
F

Failures:

  1) RailsAdmin secondary navigation does not cause error when email is nil
     Failure/Error: visit dashboard_path
     ActionView::Template::Error:
       undefined method `devise_scope' for #<User:0x007ff5a3889c30>

line-height: 1;
.navbar {
/* Application name */
.brand {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tagliala Could you change in this PR the css selector .brand in .navbar-brand please? 😄
These are stylesheets for the bootstrap2 markup.

If you prefer I could do a PR after the merge of this one.

Thanks!

@dalpo
Copy link
Contributor

dalpo commented May 27, 2015

👍

@tagliala
Copy link
Contributor Author

@dalpo thanks

Completely removed the old css rules. I didn't like that old brown label

image

@tagliala
Copy link
Contributor Author

Failures due to #2311

@dalpo
Copy link
Contributor

dalpo commented May 28, 2015

Thanks to you @tagliala! Great work 😄

@tagliala
Copy link
Contributor Author

could you please re-run the Travis build?

@mshibuya mshibuya added this to the 0.7 milestone May 29, 2015
@luizpicolo
Copy link
Contributor

This bothered me a long time 😄 . Great work 👍

@tagliala
Copy link
Contributor Author

tagliala commented Jun 2, 2015

thanks! honestly the "log out" red label is still bothering me but... one thing at a time :)

@dalpo
Copy link
Contributor

dalpo commented Jun 17, 2015

@mshibuya I see that you are planning a 0.7 milestone?
When do you think to merge these PRs?

link_to _current_user.email, url_for(action: edit_action.action_name, model_name: abstract_model.to_param, id: _current_user.id, controller: 'rails_admin/main')
link_to url_for(action: edit_action.action_name, model_name: abstract_model.to_param, id: _current_user.id, controller: 'rails_admin/main') do
html = []
html << image_tag("#{(request.ssl? ? 'https://secure' : 'http://www')}.gravatar.com/avatar/#{Digest::MD5.hexdigest _current_user.email}?s=30", alt: '') if _current_user.email.present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tagliala What do you think about to add the 'img-rounded' (or the 'img-circle') class to the avatar image?

Take a look at http://getbootstrap.com/css/#images-shapes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not anymore a big fan of circled avatars because they cut out part of the user profile picture.

btw no problems for me if authors decide to go for img-circle, I will edit this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice to go for a more modern/consolidated user drop down. I'll try and find an example, but you've probably seen them before - the ones where you click the username/image and a drop down appears with a profile and logout link. It would solve the red "danger-label" as mentioned above and tidy up the top nav bar... Just a thought!

@rikkipitt
Copy link
Contributor

@tagliala and how about a target="_blank" on the "Home" link too? That might make for a nicer experience.

@tagliala
Copy link
Contributor Author

@rikkipitt it makes sense, but I would leave this decision to the authors. No problem to edit this PR

mshibuya added a commit that referenced this pull request Jun 24, 2015
@mshibuya mshibuya merged commit 07f2e32 into railsadminteam:master Jun 24, 2015
@mshibuya
Copy link
Member

Merged in, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants