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

Hide signup path unless the instance allows new registration. #3055

Merged
merged 1 commit into from May 15, 2017

Conversation

treby
Copy link
Contributor

@treby treby commented May 14, 2017

When an instance does not allow register account ( open_registrations = false ),

Before
2017-05-14 23 31 10

After
2017-05-14 23 04 06

@ykzts
Copy link
Sponsor Member

ykzts commented May 14, 2017

What happens to languages ​​other than en and ja with this change?

@treby
Copy link
Contributor Author

treby commented May 14, 2017

@ykzts Thank you for your comment!

Actually, I've tried to add other language's keys by i18n-tasks add-missing, then there is so big diff (although not pr's related keys are added). So I wondered if is there any guidelines for new i18n key?

Of course, I'm happy to add other languages key by en translation, or just split existing landing_strip_html value by my hand (but since I'm not good at many languages, I may break existing translation...).

What can I do to improve this pr?

link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path),
sign_up_path: new_user_registration_path)
link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path))
- if open_registrations?
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It would be better to use InstancePresenter like about/_links.html.haml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm...
To make this pr, at first, I tried to use InstancePresenter to get the configuration (and DRY).
but IMHO, it's not a very good idea to provide the presenter instance only for _landing_strip.html.haml .

Now, _landing_strip.html.haml is called from

  • app/views/accounts/show.html.haml ( app/controllers/accounts_controller.rb )
  • app/views/stream_entries/show.html.haml (app/controllers/stream_entries_controller.rb )

There two controllers are completely difference (unlike about_controller), so I think it's hard for developers to know how controller's InstanceRepresenter instance variable is used.

Of course, I'll add both controllers by your request, but my opinion is above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an patch image for your request (that I'm thinking).

diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb
index 8eda9633..319955cf 100644
--- a/app/controllers/accounts_controller.rb
+++ b/app/controllers/accounts_controller.rb
@@ -6,6 +6,7 @@ class AccountsController < ApplicationController
   def show
     respond_to do |format|
       format.html do
+        @instance_presenter = InstancePresenter.new
         @statuses = @account.statuses.permitted_for(@account, current_account).order('id desc').paginate_by_max_id(20, params[:max_id], params[:since_id])
         @statuses = cache_collection(@statuses, Status)
       end
diff --git a/app/controllers/stream_entries_controller.rb b/app/controllers/stream_entries_controller.rb
index a9ee7350..64277566 100644
--- a/app/controllers/stream_entries_controller.rb
+++ b/app/controllers/stream_entries_controller.rb
@@ -13,6 +13,7 @@ class StreamEntriesController < ApplicationController
       format.html do
         return gone if @stream_entry.activity.nil?
 
+        @instance_presenter = InstancePresenter.new
         if @stream_entry.activity_type == 'Status'
           @ancestors   = @stream_entry.activity.reply? ? cache_collection(@stream_entry.activity.ancestors(current_account), Status) : []
           @descendants = cache_collection(@stream_entry.activity.descendants(current_account), Status)
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 8f1cd8fc..92ffac33 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -9,10 +9,6 @@ module ApplicationHelper
     !user_signed_in? && !single_user_mode?
   end
 
-  def open_registrations?
-    Setting.open_registrations
-  end
-
   def add_rtl_body_class(other_classes)
     other_classes = "#{other_classes} rtl" if [:ar, :fa, :he].include?(I18n.locale)
     other_classes
diff --git a/app/views/accounts/show.html.haml b/app/views/accounts/show.html.haml
index a1904910..e21827da 100644
--- a/app/views/accounts/show.html.haml
+++ b/app/views/accounts/show.html.haml
@@ -9,7 +9,7 @@
   = render 'og', account: @account, url: short_account_url(@account, only_path: false)
 
 - if show_landing_strip?
-  = render partial: 'shared/landing_strip', locals: { account: @account }
+  = render partial: 'shared/landing_strip', locals: { account: @account, instance_presenter: @instance_presenter }
 
 .h-feed
   %data.p-name{ value: "#{@account.username} on #{site_hostname}" }/
diff --git a/app/views/shared/_landing_strip.html.haml b/app/views/shared/_landing_strip.html.haml
index 3cc61a2c..77b2678a 100644
--- a/app/views/shared/_landing_strip.html.haml
+++ b/app/views/shared/_landing_strip.html.haml
@@ -2,5 +2,5 @@
   = t('landing_strip_html',
     name: content_tag(:span, display_name(account), class: :emojify),
     link_to_root_path: link_to(content_tag(:strong, site_hostname), root_path))
-  - if open_registrations?
+  - if instance_presenter&.open_registrations
     = t('landing_strip_signup_html', sign_up_path: new_user_registration_path)
diff --git a/app/views/stream_entries/show.html.haml b/app/views/stream_entries/show.html.haml
index d01e82af..8ba16b78 100644
--- a/app/views/stream_entries/show.html.haml
+++ b/app/views/stream_entries/show.html.haml
@@ -13,7 +13,7 @@
   %meta{ property: 'twitter:card', content: 'summary' }/
 
 - if show_landing_strip?
-  = render partial: 'shared/landing_strip', locals: { account: @stream_entry.account }
+  = render partial: 'shared/landing_strip', locals: { account: @stream_entry.account, instance_presenter: @instance_presenter }
 
 .activity-stream.activity-stream-headless.h-entry
   = render partial: "stream_entries/#{@type}", locals: { @type.to_sym => @stream_entry.activity, include_threads: true }

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Well surely it is.

@ykzts
Copy link
Sponsor Member

ykzts commented May 14, 2017

I think that it is okay to temporarily make it into English.

@ykzts
Copy link
Sponsor Member

ykzts commented May 14, 2017

Of course you should divide within the range you can split.

@treby
Copy link
Contributor Author

treby commented May 15, 2017

Okay I'll try it.

@treby
Copy link
Contributor Author

treby commented May 15, 2017

@ykzts Added all language's translation and replied your review

@Gargron Gargron merged commit cb50ecd into mastodon:master May 15, 2017
@treby treby deleted the split-signup-html branch May 15, 2017 23:35
Gargron added a commit that referenced this pull request May 22, 2017
Gargron added a commit that referenced this pull request May 22, 2017
Gargron added a commit that referenced this pull request May 23, 2017
Gargron added a commit that referenced this pull request May 23, 2017
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.

None yet

3 participants