Split admin settings #2650

aamarill · 2018-05-24T00:05:03Z

@bertocq @decabeza this is a fix for issue #2586 😄

Objectives

Break up the "settings" menu into different tabs for better visualization.

Visual Changes

Screenshots

Before/was:

After/is:

After/is (Cont.)

Notes

This change was implemented using the views structure from the "Admin/Polls/Poll Questions" menu.

voodoorai2000

Marvellous! 👏

decabeza · 2018-05-24T12:17:25Z

app/views/admin/settings/_banner_images.html.erb

+ </tbody>
+ </table>
+<% else %>
+ <h3>No banner images to show.</h3>


Can you change hardcoded text creating a new i18n key <%= t("admin.settings.index.no_banners_images") %> 🙏

decabeza · 2018-05-24T12:17:37Z

app/views/admin/settings/_banner_styles.html.erb

+ </tbody>
+ </table>
+<% else %>
+ <h3>No banner styles to show.</h3>


Can you change hardcoded text creating a new i18n key <%= t("admin.settings.index.no_banners_styles") %>

decabeza · 2018-05-24T12:18:05Z

app/views/admin/settings/_feature_flags.html.erb

@@ -0,0 +1,28 @@
+


Remove extra line 🙏

Oh and don't forget to remove this extra line on a new commit, @aamarill

aitbw · 2018-05-24T14:56:21Z

Hey @aamarill, thanks for this! Just two small things:

Could you please rebase your branch with master so we can get @raul-fuentes' commits out of this PR?
Could you also squash the last 2 commits into one? Since they're basically the same

Thanks once again! ☺️

aamarill · 2018-05-24T15:20:17Z

@aitbw No problem!

Can you please clarify your bullet points. What branch exactly (repo and branch) are you referring to when you say "your branch"? I pulled the latest from "consul/consul master branch" before I did my second commit. Is that not a standard practice?

Also, how do I squash two commits together?

Thanks! 😄

aitbw · 2018-05-24T15:37:10Z

Sure 😌

When I say your branch, I'm referring to the one called 2586-split_admin_settings, which is located on your CONSUL fork

I would't know if pulling from master directly into your branch (and creating a merge commit with it) is a standard practice but I tend to avoid it since it includes commits which are not related to your proposed changes, hence making the PR harder to review

If I need to update my branch with the latest master revision, I usually do the following:

git checkout master
git pull origin master
git checkout your-branch
git rebase origin/master

You can also simplify this by doing (while being on your branch with your proposed changes) the following: git pull origin master --rebase, then do git push --force and you're all set 😸

To squash two commits together, do the following:

git rebase -i HEAD~2

You'll be greeted with something like this:

Keep the first line as it is and replace pick on the second line with either squash or fixup, the latter is even better since you'll be omitting the commit message of the latest commit on your branch (which is exactly the same as the previous commit)

aamarill · 2018-05-24T16:20:44Z

@aitbw Thank you so much for the help so far!

I am running into trouble because I rebased my local master and fork master with consul/master... How can I get the master branch that I initially started with? Thanks!

aitbw · 2018-05-24T16:23:34Z

@aamarill That would mean a hard reset and resetting the master branch is a no go. Are you sure you want to do that?

Also, which issues are you currently dealing with?

aamarill · 2018-05-24T16:27:51Z

Could you please rebase your branch with master so we can get @raul-fuentes' commits out of this PR?

@aitbw The issue is that I don't have a branch that doesn't include the commits you want me to remove . (I am not referring to any code failures/issues) 😕

aitbw · 2018-05-24T16:34:59Z

@aamarill When I asked you to rebase your branch to get @raul-fuentes' commits out of this PR, I meant the Commits and Files changed tabs here on GitHub

Of course said commits will be on your branch, they just won't show up as changes for your Pull Request

aamarill · 2018-05-24T16:44:08Z

@aitbw Ah! I do apologize for all of the confusion, I am quite new to the git/GitHub workflow. I don't see an option to remove the commits and files from @raul-fuentes ... I am not sure if I don't have the privileges or am I not looking in the right place? Thanks!! 😄

Here's what I see on my end:

Hope that helps!

aitbw · 2018-05-24T16:47:06Z

@aamarill Don't worry about it, we all start somewhere ☺️

If you ran on your branch git pull origin master --rebase and everything went smoothly, you need to do git push origin your-branch-name --force so the remote (as well as the PR) can be updated ☺️

…und by id

aamarill · 2018-05-24T17:00:06Z

@aitbw Thank you for your patience 🙏

Ok, I did have to do some conflict resolution, but I was able to rebase and then push back to my fork and hence update the PR. Let me know if it looks good!

aitbw · 2018-05-24T17:03:18Z

@aamarill Don't even mention it ☺️

And yes, it looks awesome!! Now only your changes are reflected on the PR making it easier to review 🎉

Let's wait for Travis now 😸

aamarill · 2018-05-24T18:44:14Z

This following test is failing because I added:

<h3><%= t("admin.settings.index.no_banners_styles") %></h3>
and
<h3><%= t("admin.settings.index.no_banners_images") %></h3>
to their respective partials. Any ideas?

test that is failing:

require 'i18n/tasks'

RSpec.describe 'I18n' do
  let(:i18n) { I18n::Tasks::BaseTask.new }
  let(:missing_keys) { i18n.missing_keys }
  let(:unused_keys) { i18n.unused_keys }

  it 'does not have missing keys' do
    expect(missing_keys).to be_empty,
      "Missing #{missing_keys.leaves.count} i18n keys, run `i18n-tasks missing' to show them"
  end

  ...
end

failure

 1) I18n does not have missing keys
     Failure/Error:
       expect(missing_keys).to be_empty,
         "Missing #{missing_keys.leaves.count} i18n keys, run `i18n-tasks missing' to show them"
     
       Missing 2 i18n keys, run `i18n-tasks missing' to show them
     # ./spec/i18n_spec.rb:10:in `block (2 levels) in <top (required)>'

Output of `$: i18n-tasks missing`

~/Bloc/Module_6/consul $ i18n-tasks missing
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.2.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
NOTE: Gem.gunzip is deprecated; use Gem::Util.gunzip instead. It will be removed on or after 2018-12-01.
Gem.gunzip called from /Users/adanamarillas/.rvm/gems/ruby-2.3.2/gems/unicode-display_width-1.3.0/lib/unicode/display_width/index.rb:5.
Missing translations (2) | i18n-tasks v0.9.20
+--------+----------------------------------------+-----------------------------------------------------+
| Locale | Key                                    | Value in other locales or source                    |
+--------+----------------------------------------+-----------------------------------------------------+
|  all   | admin.settings.index.no_banners_images | app/views/admin/settings/_banner_images.html.erb:23 |
|  all   | admin.settings.index.no_banners_styles | app/views/admin/settings/_banner_styles.html.erb:23 |
+--------+----------------------------------------+-----------------------------------------------------+

aitbw · 2018-05-24T18:56:27Z

@aamarill That is because you referenced the keys on the views but forgot to add them under config/locales/en and config/locales/es

aamarill · 2018-05-24T19:11:10Z

@aitbw Fixed! I am moving on to the last two tests that are failing.
I actually do speak Spanish, but I am not sure which translation is the best for:

no_banners_images: No banner images
no_banners_styles: No banner styles

FYI, these don't have a Spanish translation either:

banners: Banner style
banner_imgs: Banner images

aitbw · 2018-05-24T19:19:19Z

For the no_banners_images and no_banners_images keys, I think we'd need @decabeza's output here

For the keys that don't have a translation (we tend to miss those from time to time), feel free to add a commit as well fixing that 😸

…tings

decabeza · 2018-05-25T17:30:46Z

Don't worry, the translation is perfect! 👌 😉

aamarill · 2018-05-25T18:04:33Z

@decabeza Awesome! 🥂 Is there anything else we need before accepting/merging this PR? 😄

PierreMesure · 2018-06-20T07:51:14Z

Hey @aamarill! Really nice work you did here! I just wanted to warn you that updating a field in any of the tabs will always send you back to the first tab. It would be nice if it could stay on the tab after you update.

Also, I'm often having a bug on Safari macOS (latest): the content of the tabs doesn't appear when I switch. I doesn't happen on Chrome.

aamarill · 2018-06-22T18:56:49Z

Thanks for the note @PierreMesure!
Unfortunately, I don't foresee having time to fix this in the near future.
Feel free to open an issue for it if you would like for someone to work on it.
It may even be a good "first-timer" contribution. Cheers!

voodoorai2000 approved these changes May 24, 2018

View reviewed changes

decabeza reviewed May 24, 2018

View reviewed changes

decabeza approved these changes May 24, 2018

View reviewed changes

Adan Amarillas added 4 commits May 24, 2018 09:55

Add 'settings' link to side menu on Admin's panel.

b59af89

Split configuration settings into tabs

7f0bfd4

Adjust tests to new navigation. Add id tag to map tab so it can be fo…

2c5cefc

…und by id

Make tabs i18n-friendly

60bc3f6

Adan Amarillas added 2 commits May 24, 2018 12:27

Add missing i18n keys to locales

1d974c4

Modify tests to reflect the new navigation on admin configuration set…

361518b

…tings

voodoorai2000 assigned aamarill May 24, 2018

voodoorai2000 added this to Doing in Roadmap May 24, 2018

voodoorai2000 removed this from Doing in Roadmap May 24, 2018

decabeza changed the title ~~2586 split admin settings~~ Split admin settings Jun 1, 2018

decabeza merged commit ec58ada into consuldemocracy:master Jun 1, 2018

decabeza mentioned this pull request Jun 4, 2018

Upstream AyuntamientoMadrid/consul#1506

Merged

voodoorai2000 mentioned this pull request Jun 12, 2018

Split Admin Settings menu into submenus #2586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split admin settings #2650

Split admin settings #2650

aamarill commented May 24, 2018 •

edited

Loading

voodoorai2000 left a comment

decabeza May 24, 2018

decabeza May 24, 2018

decabeza May 24, 2018

aitbw May 24, 2018

aamarill May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018 •

edited

Loading

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

decabeza commented May 25, 2018

aamarill commented May 25, 2018

PierreMesure commented Jun 20, 2018

aamarill commented Jun 22, 2018

Split admin settings #2650

Split admin settings #2650

Conversation

aamarill commented May 24, 2018 • edited Loading

Objectives

Visual Changes

Before/was:

After/is:

After/is (Cont.)

After/is (Cont.)

Notes

voodoorai2000 left a comment

Choose a reason for hiding this comment

decabeza May 24, 2018

Choose a reason for hiding this comment

decabeza May 24, 2018

Choose a reason for hiding this comment

decabeza May 24, 2018

Choose a reason for hiding this comment

aitbw May 24, 2018

Choose a reason for hiding this comment

aamarill May 24, 2018

Choose a reason for hiding this comment

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018 • edited Loading

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

Here's what I see on my end:

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

aamarill commented May 24, 2018

test that is failing:

failure

Output of $: i18n-tasks missing

aitbw commented May 24, 2018

aamarill commented May 24, 2018

aitbw commented May 24, 2018

decabeza commented May 25, 2018

aamarill commented May 25, 2018

PierreMesure commented Jun 20, 2018

aamarill commented Jun 22, 2018

aamarill commented May 24, 2018 •

edited

Loading

aitbw commented May 24, 2018 •

edited

Loading

Output of `$: i18n-tasks missing`