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

Show instance name at login screen #957

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

kidhab
Copy link
Contributor

@kidhab kidhab commented Oct 13, 2022

This commit is useful for multi_coop_installs. At the moment the login screen of every multi_coop instance looks the same.
It can happen that users log in to another instance because they mistyped the URL.
With this change to name of a foodsoft instance will be shown as the title of the login screen.

Copy link
Member

@paroga paroga left a comment

Choose a reason for hiding this comment

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

IMHO the message is misleading, since the name is already shown in the bottom left of the login screen

is there a reason why this is not configurable via the setting pages?

@@ -4,7 +4,10 @@
$('#nick').focus();
});

- title t('.title')
- if FoodsoftConfig[:instance_name].present?
- title FoodsoftConfig[:instance_name]
Copy link
Member

Choose a reason for hiding this comment

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

why not FoodsoftConfig[:instance_name] || t('.title') to avoid duplication of the config access?

what's the benefit of instance_name over the already existing name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no benefit if every foodcoop changes their foodcoop's name at the configuration page. Actually a lot of foodcoops didn't do it. But maybe this is a special case at our hosting platform.
As you suggested it would be better to use FoodsoftConfig:[:name] to make it editable by users.

Comment on lines 170 to 176
# Multi_coop example configuration
# In order to make this work set "multi_coop_install: true"
#anotherfoodcoop:
# <<: *defaults
# database:
# database: foodsoft_anotherfoodcoop
# instance_name: anotherfoodcoop
Copy link
Member

Choose a reason for hiding this comment

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

please move it to the other options like name, since it's not more or less special

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 think this options have to be outside the default configuration block to make it work.

@kidhab kidhab force-pushed the show_instance_name branch from 57f1b79 to c508a3e Compare October 13, 2022 19:00
Comment on lines 170 to 176
# Multi_coop example configuration
# In order to make this work set "multi_coop_install: true"
#anotherfoodcoop:
# <<: *defaults
# database:
# database: foodsoft_anotherfoodcoop

Copy link
Member

Choose a reason for hiding this comment

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

what's the benefit of adding this lines? IMO everything is already present in the file.

@@ -4,7 +4,7 @@
$('#nick').focus();
});

- title t('.title')
- title FoodsoftConfig[:name] || t('.title')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really happy with this. I'd prefer to keep the t('.title') in some form on the login page and use something like t('.title', FoodSoftConfig[:name]) to pass the name to the translation file, but this will probably result in (too) long lines. what you think about just adding something like h2 FoodsoftConfig[:name] after the title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Indeed it looks better now with a subtitle.

@kidhab kidhab force-pushed the show_instance_name branch from c508a3e to 36dc095 Compare October 28, 2022 16:07
@kidhab kidhab force-pushed the show_instance_name branch from 36dc095 to 33e3963 Compare October 28, 2022 16:09
@kidhab kidhab merged commit 8420323 into foodcoops:master Mar 29, 2023
@kidhab kidhab deleted the show_instance_name branch March 29, 2023 14:01
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.

2 participants