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

Fix: edit_user_link rendering #3475

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Conversation

thedanielhanke
Copy link
Contributor

@thedanielhanke thedanielhanke commented Feb 22, 2022

if config.show_gravatar is e.g. false, "false" becomes part of the render output as false is not nil and therefore not removed on #compact

if there is no edit-user action to be found, the rendered output was 2 nested spans, which messed up layout.

@thedanielhanke thedanielhanke changed the title Fix: edit_user_link Fix: edit_user_link rendering Feb 22, 2022
@coveralls
Copy link

coveralls commented Feb 22, 2022

Coverage Status

coverage: 95.738%. first build
when pulling b0c5351 on thedanielhanke:patch-1
into 50d34c9 on railsadminteam:master.

@mshibuya
Copy link
Member

Would you mind adding some tests for this fix? They should fit into somewhere around here:
https://github.com/railsadminteam/rails_admin/blob/master/spec/helpers/rails_admin/application_helper_spec.rb#L463-L506

@thedanielhanke
Copy link
Contributor Author

thedanielhanke commented Feb 23, 2022

@mshibuya

concerning the tests: sure, the only issue i had was getting rspec up and running when using sprockets.

it failed at require sprockets/railstie - is this a known thing? will try webpacker later.

as rspec did not start and the current specs just dont really care for the generated html, my first attempt was just to quick-fix the rendering issue.

would love to add some actual improvements (i owe you :))

@mshibuya
Copy link
Member

Ah you need to run rspec via appraisal, like this:

$  appraisal rails-7.0 rspec spec/helpers/rails_admin/application_helper_spec.rb

@thedanielhanke
Copy link
Contributor Author

thanks, working now.

question concerning "#{request.ssl? ? 'https://secure' : 'http://www'}.":

is there any real world requirement for the http://www part? why not always do https here?

if show_gravatar is e.g. `false`, "false" becomes part of the render output sa `false` is not removed on `.compact`


if there is no edit-user action to be found, the output was 2 nested spans, which messed up layout.
@thedanielhanke
Copy link
Contributor Author

thedanielhanke commented Feb 23, 2022

@mshibuya
i did some changes, please have a look.

paused now, as i changed quite a bit - if you are not entirely against these kind of changes, i`ll continue.
kind of changes: imo. readability, maintainability, specs care a bit more about the actual outcome.

the 'include Pundit::Authorization' commit is not PR-related, i can move it to another branch later too.

@mshibuya
Copy link
Member

mshibuya commented Feb 24, 2022

@thedanielhanke

kind of changes: imo. readability, maintainability, specs care a bit more about the actual outcome.

Please don't do it now. You can propose these changes in a separate PR, but do not mix it with the issue fix.

(And I personally think your style of writing specs has more downsides, it's less descriptive and needs more execution time. But this is not relevant here anyway...)

@mshibuya mshibuya merged commit f92bdc5 into railsadminteam:master Feb 25, 2022
@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.

3 participants