Rename edit enterprise page to "Settings" #2515

kristinalim · 2018-08-03T12:00:59Z

What? Why?

This improves and standardizes the name of the page for editing an enterprise. This will make communication internally and with users easier. Please see related issue for more details.

What should we test?

As enterprise owner:

In the admin section, go to "Enterprises".
Check the "Manage" column. It should have "Settings" buttons instead of "Manage". See the following image:
Click on "Settings". This should lead to the main section of the page for editing the enterprise.
Check the heading of the page. It should start with "Settings:" instead of "Editing:".

As admin user:

In the admin section, go to "Enterprises".
See the list of actions for an enterprise. It should have a "Settings" link instead of "Edit Profile". There should also be a separator below the "Delete" link. It should look like the following image:
Click on "Settings". This should lead to the main section of the page for editing the enterprise.
Check the heading of the page. It should start with "Settings:" instead of "Editing:".

Release notes

Standardize name of page for editing enterprise to "Settings".

Changelog Category: Changed

Matt-Yorkley · 2018-08-03T12:15:28Z

app/assets/stylesheets/admin/enterprises.css.scss

+  margin-top: 1em;
+  margin-bottom: 1em;
+  height: 1px;
+  background-color: $admin-enterprises-index__admin-actions-divider-color;


The new color you've defined here has the same value as the existing $pale-blue variable, so you can just use: background-color: $pale-blue.

Matt-Yorkley · 2018-08-03T12:16:31Z

app/assets/stylesheets/admin/variables.css.scss

@@ -8,3 +8,5 @@ $warning-red: #da5354;
 $warning-orange: #da7f52;
 $medium-grey: #919191;
 $pale-blue: #cee1f4;
+
+$admin-enterprises-index__admin-actions-divider-color: #cee1f4;


...and this can be deleted.

@Matt-Yorkley Agh. Thanks, I was switching between colors and didn't realize that the final one I picked was declared as a variable immediately above.

What do you think about still using the variable for the divider color, but setting it $pale-blue, like this:

$admin-enterprises-index__admin-actions-divider-color: $pale-blue;

In my experience, separating colors would make it easier to make color changes site-wide. Especially for OFN which is multi-instance, I imagine it would be convenient if more colors were assigned like this.

I don't think the variable for $admin-enterprises-index__admin-actions-divider-color needs to be shared throughout all the SCSS, as it's a single element on a single page.

I'm not sure about the complex name either, it seems much less likely to be re-used elsewhere if it's named by the exact page it's in.

I know what you mean though. I think as the colour is the same as the border colour for tables, I think I'd probably add an entry like this with the main colours:

$admin-table-border: $spree-blue;

then use that on your divider element:

background-color: $admin-table-border;

Matt-Yorkley · 2018-08-06T08:30:43Z

app/views/admin/enterprises/_actions.html.haml

@@ -5,6 +5,8 @@
  = link_to_delete_enterprise enterprise
  %br/

+<div class="admin-enterprises-index__admin-actions-divider"></div>
+


Would a horizontal rule (hr) element not be better here? Or is there a reason to use a div with height 1px?

Also, we don't need all these braces and closing bracket in haml. %div.classname is fine, or as a div is the most common element you can skip it all together and just use .classname as a shorthand.

kristinalim · 2018-08-06T11:13:11Z

Thanks for your review, @Matt-Yorkley! I absolutely agree and have made changes.

This PR now uses HR, preferring that over DIV for better accessibility and semantics.

I also switched from a BEM-style CSS class name to keep scss-lint from complaining. Although I'm considering proposing to start using BEM conventions for the project.

mkllnk

Great.

daniellemoorhead · 2018-08-07T06:54:09Z

Heya @kristinalim could you please make sure you assign yourself to the PRs you create. It makes it easier for us delivery train drivers to see who's doing what. Thanks!

kristinalim · 2018-08-07T07:13:15Z

I'm not able to assign myself to PRs I create either. 🙁 I'm working on getting certified though. If/when that happens, I would be able to ask for more permissions.

kirstenalarsen · 2018-08-07T08:10:11Z

not sure if I'm violating a process here but I trust you @kristinalim (obviously) so have added you to the repo :)

sigmundpetersen · 2018-08-07T08:29:59Z

The process is so far described in the contributor's guide. Since you are one of the facilitators to contact for certification requests @kirstenalarsen it seems you are not violating anything 😉

kristinalim · 2018-08-07T08:50:49Z

Thank you, @kirstenalarsen! 🙂 I've assigned to myself all PRs I submitted and all issues I've been working on.

Matt-Yorkley

Great! 🙂

daniellemoorhead · 2018-08-08T05:45:33Z

I've moved this to test ready as it's been reviewed twice. @Matt-Yorkley don't forget to move it along the train if you're the second reviewer, and also stage it if you can!

The ".manage" key is still used by the column header for the "Manage" buttons.

Change the start of the heading from "Editing:" to "Settings:".

Change "Edit Profile" link to "Settings".

mkllnk · 2018-08-10T02:27:02Z

@kristinalim I resolved a merge conflict with some of my recent spec changes.

Staged on https://staging1.openfood.com.au/.

sstead · 2018-08-10T04:44:45Z

Testing notes

Reads settings now, for enterprise users and superadmin :) yay

https://docs.google.com/document/d/1K8V51UZTR40wpRyjudN02XsjUSJ6vC7OnnxVVeyLl2k/edit#

kristinalim force-pushed the translations-rename_enterprise_profile_to_settings branch from b31c76b to 9d1f0ea Compare August 3, 2018 12:06

Matt-Yorkley reviewed Aug 3, 2018

View reviewed changes

kristinalim force-pushed the translations-rename_enterprise_profile_to_settings branch from 9d1f0ea to 88e57e5 Compare August 5, 2018 13:45

Matt-Yorkley reviewed Aug 6, 2018

View reviewed changes

kristinalim force-pushed the translations-rename_enterprise_profile_to_settings branch 3 times, most recently from 0dd30a3 to 216750c Compare August 6, 2018 11:06

mkllnk approved these changes Aug 7, 2018

View reviewed changes

sigmundpetersen assigned kristinalim Aug 7, 2018

Matt-Yorkley approved these changes Aug 7, 2018

View reviewed changes

kristinalim added 5 commits August 10, 2018 12:12

Split translation key for enterprise manage button

b91019a

The ".manage" key is still used by the column header for the "Manage" buttons.

Change enterprise "Manage" button to "Settings"

0b4fcf8

Change heading for edit enterprise page

295604b

Change the start of the heading from "Editing:" to "Settings:".

Rename edit link in admin enterprise index

17978f5

Change "Edit Profile" link to "Settings".

Add divider between admin enterprise index actions

6251c72

mkllnk force-pushed the translations-rename_enterprise_profile_to_settings branch from 216750c to 6251c72 Compare August 10, 2018 02:17

oeoeaio merged commit 6251c72 into openfoodfoundation:master Aug 10, 2018

kristinalim deleted the translations-rename_enterprise_profile_to_settings branch August 30, 2018 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename edit enterprise page to "Settings" #2515

Rename edit enterprise page to "Settings" #2515

kristinalim commented Aug 3, 2018

Matt-Yorkley Aug 3, 2018

Matt-Yorkley Aug 3, 2018

kristinalim Aug 5, 2018

Matt-Yorkley Aug 6, 2018

Matt-Yorkley Aug 6, 2018

Matt-Yorkley Aug 6, 2018

kristinalim commented Aug 6, 2018

mkllnk left a comment

daniellemoorhead commented Aug 7, 2018

kristinalim commented Aug 7, 2018

kirstenalarsen commented Aug 7, 2018

sigmundpetersen commented Aug 7, 2018

kristinalim commented Aug 7, 2018

Matt-Yorkley left a comment •

edited

daniellemoorhead commented Aug 8, 2018

mkllnk commented Aug 10, 2018

sstead commented Aug 10, 2018

Rename edit enterprise page to "Settings" #2515

Rename edit enterprise page to "Settings" #2515

Conversation

kristinalim commented Aug 3, 2018

What? Why?

What should we test?

Release notes

Matt-Yorkley Aug 3, 2018

Choose a reason for hiding this comment

Matt-Yorkley Aug 3, 2018

Choose a reason for hiding this comment

kristinalim Aug 5, 2018

Choose a reason for hiding this comment

Matt-Yorkley Aug 6, 2018

Choose a reason for hiding this comment

Matt-Yorkley Aug 6, 2018

Choose a reason for hiding this comment

Matt-Yorkley Aug 6, 2018

Choose a reason for hiding this comment

kristinalim commented Aug 6, 2018

mkllnk left a comment

Choose a reason for hiding this comment

daniellemoorhead commented Aug 7, 2018

kristinalim commented Aug 7, 2018

kirstenalarsen commented Aug 7, 2018

sigmundpetersen commented Aug 7, 2018

kristinalim commented Aug 7, 2018

Matt-Yorkley left a comment • edited

Choose a reason for hiding this comment

daniellemoorhead commented Aug 8, 2018

mkllnk commented Aug 10, 2018

sstead commented Aug 10, 2018

Matt-Yorkley left a comment •

edited