Add icon link helper by ItsGenis · Pull Request #1155 · decidim/decidim

ItsGenis · 2017-03-21T10:17:40Z

🎩 What? Why?

This adds a helper to create icon links in a more easy way, right now it is quite tedious to add an icon link. This should improve the dev experience.

I intend to update all the existing icons in the admin in this PR as well.

📌 Related Issues

Related to Admin panel refactor #1101

Use helper in existing links
Add methods
Add optional data

👻 GIF

mrcasals · 2017-03-21T10:29:32Z

@Dor3nz tests fail :(

codecov-io · 2017-03-21T11:08:18Z

Codecov Report

Merging #1155 into revamp_admin will increase coverage by <.01%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           revamp_admin    #1155      +/-   ##
================================================
+ Coverage         97.11%   97.11%   +<.01%     
================================================
  Files               421      422       +1     
  Lines              7042     7048       +6     
================================================
+ Hits               6839     6845       +6     
  Misses              203      203

Impacted Files	Coverage Δ
...dmin/app/helpers/decidim/admin/icon_link_helper.rb	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19d290b...d3fd872. Read the comment docs.

mention-bot · 2017-03-21T13:15:00Z

@Dor3nz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @josepjaume and @beagleknight to be potential reviewers.

josepjaume · 2017-03-21T13:32:29Z

decidim-admin/app/views/decidim/admin/features/_feature.html.erb

-      <%= link_to manage_feature_path(participatory_process, feature), data: { tooltip: true, disable_hover: false }, title: t("actions.manage", scope: "decidim.admin"), class: "action-icon action-icon--manage" do %>
-        <%= icon "pencil" %>
-      <% end %>
+      <%= icon_link_to "pencil", manage_feature_path(participatory_process, feature), t("actions.manage", scope: "decidim.admin"), "action-icon--manage" %>


You're using the argument klass in the wrong way, as this would actually generate action-iconaction-icon--manage. Same applies for the rest of its usage.

Actually, there's a space to split them. Check how the helper merges the classes.

Oh, nice xD. Anyway, I think specifying the whole class might be a bit redundant. Is there any case where you need to override the whole class name? If that's the case, maybe you could use a class: "whatever" option instead of this? Not a big fan of long lists of arguments when some of them are/could be optional.

For testing purposes apparently we need to "classify" them all, can turn it to optional anyways.

Oh, I understand. I'd make it optional via a options argument then.

josepjaume · 2017-03-21T15:06:43Z

Sorry for the trolling @Dor3nz: I'd make klass, method and data optional with sane defaults via a options argument. It makes for a nicer API and it'd be quite symmetrical to the way rails does things.

It's important to get these APIs right, as other developers will be using them from external modules and so they might become difficult to deprecate.

Thoughts @oriolgual @mrcasals @beagleknight ?

josepjaume · 2017-03-21T15:07:32Z

To make it clearer, the signature of the method would become:

def icon_link_to(icon_name, link, title, options = {})
  # ...
end

ItsGenis · 2017-03-21T15:14:13Z

Yeah I'm up for that.

oriolgual · 2017-03-22T08:08:56Z

decidim-admin/app/helpers/decidim/admin/icon_link_helper.rb

+module Decidim
+  module Admin
+    module IconLinkHelper
+      def icon_link_to(icon_name, link, title, klass, method, data={})


mrcasals · 2017-03-22T08:10:13Z

@Dor3nz be careful when merging this options hash with the data param of the link_to method.

mrcasals · 2017-03-22T09:14:23Z

decidim-admin/app/helpers/decidim/admin/icon_link_helper.rb

+      #
+      def icon_link_to(icon_name, link, title, options = {})
+        link_to(link,
+                method: options[:method],


I'd default :method to :get, just in case, as it's the default value for the link_to method.

method: options[:method] || :get

It does so already when nil.

Weird, but ok.

But remove the method: :get calls then, if it's the default value.

* Add icon link helper * Update existing icon links * Add optional data to icon link helper * Add methods at icon links * Refactor tests for new icon helper * Add docs and refactor helper call * Refactor all icon links with new helper call

* Add admin styles * Add basic layout * Change scopes icon * Add participatory processes index page * Add participatory process layout * Fix some specs * Fix more broken tests * Fix more specs * Fix poltergeist * New admin - Settings section (#1124) * Add participatory process layout * Add basic layout for settings * Finish settings form * Add scopes index page * Finish basic scopes admin * Add revamp_admin to travis * Fix a few specs * Fix broken tests * Remove bad merge * [New admin] - Pages section (#1130) * Add index section * Add new section * Finish forms * Fix specs * [New admin] - Users (#1136) * Add layout * Add users index page * Add invite user form * Add user groups index and fix pagination for both users and user groups * Fix table links * Fix specs * Fix admin invite user labels * [New admin] - Newsletters (#1140) * Add layout and index page * Add new/edit forms * Add newsletter preview * Fix specs * [New Admin] - Process groups (#1141) * Add new views * Fix broken specs * [New Admin] - Style process features (#1143) * Style process features * Refactor feature tests for admin * [New Admin] - Processes info / steps (#1138) * updated new process view * updated process steps index * update process steps index table * update processes layouts * Processes steps views updated * Fix indentation * Remove unusued locales * Update tests * Add moderations missing menu * missing td added * Add icon link helper (#1155) * Add icon link helper * Update existing icon links * Add optional data to icon link helper * Add methods at icon links * Refactor tests for new icon helper * Add docs and refactor helper call * Refactor all icon links with new helper call * [New Admin] Update category views. (#1148) * Add category views * Fix tests * Add tests * Add new icon_helper * [New Admin] Attachment update. (#1158) * Views added * Finish tests * add helper * Style process features at admin (#1149) * [New Admin] - Process groups (#1141) * Add new views * Fix broken specs * Style meetings table * Update branch with icon helper * Style proposals * Style results * Style budgets * Update missing styles for some forms * Fix most of the tests * Fix more tests * Style moderation tables (#1182) * Remove unused travis step * [New Admin] Update process admins views. (#1183) * Add new and edit forms * clean update command * Tests added * Amend test * Add dashboard content * Spellcheck * Amend managing roles strings * Use user instead of admin * Fix participatory process edit button * Add some feedback * Add styles for feature forms * Add edit feature form * Add more feedback * [New Admin] Html and styles review. (#1200) * Review all the forms * Class coherence review * CA: Swap funcionalitat with component * Fix a few specs * ES: Swap funcionalidad with componente * Fix a few more specs * Add missing helper * Add preview images to organization settings form * Icons updated on features table * amend icon

ItsGenis added the in-progress label Mar 21, 2017

mrcasals approved these changes Mar 21, 2017

View reviewed changes

beagleknight approved these changes Mar 21, 2017

View reviewed changes

beagleknight force-pushed the revamp_admin branch from b984416 to 19d290b Compare March 21, 2017 11:00

ItsGenis changed the title ~~Add icon link helper~~ Add icon link helper [WIP] Mar 21, 2017

ItsGenis added 2 commits March 21, 2017 12:17

Add icon link helper

9855bde

Update existing icon links

45565db

ItsGenis force-pushed the feature/add-icon-helper branch from 409f520 to 45565db Compare March 21, 2017 11:17

ItsGenis added 2 commits March 21, 2017 12:36

Add optional data to icon link helper

9adccb7

Add methods at icon links

2f79444

ItsGenis changed the title ~~Add icon link helper [WIP]~~ Add icon link helper Mar 21, 2017

ItsGenis changed the title ~~Add icon link helper~~ Add icon link helper [WIP] Mar 21, 2017

ItsGenis added in-review and removed in-progress labels Mar 21, 2017

ItsGenis changed the title ~~Add icon link helper [WIP]~~ Add icon link helper Mar 21, 2017

josepjaume suggested changes Mar 21, 2017

View reviewed changes

Refactor tests for new icon helper

2d69da5

oriolgual suggested changes Mar 22, 2017

View reviewed changes

Add docs and refactor helper call

49633f0

mrcasals suggested changes Mar 22, 2017

View reviewed changes

Refactor all icon links with new helper call

d3fd872

ItsGenis force-pushed the feature/add-icon-helper branch from 702fc14 to d3fd872 Compare March 22, 2017 09:24

mrcasals approved these changes Mar 22, 2017

View reviewed changes

oriolgual approved these changes Mar 22, 2017

View reviewed changes

josepjaume approved these changes Mar 22, 2017

View reviewed changes

josepjaume merged commit 0e2e2df into revamp_admin Mar 22, 2017

josepjaume deleted the feature/add-icon-helper branch March 22, 2017 09:55

josepjaume removed the in-review label Mar 22, 2017

Uh oh!

Conversation

ItsGenis commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

👻 GIF

Uh oh!

mrcasals commented Mar 21, 2017

Uh oh!

codecov-io commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mention-bot commented Mar 21, 2017

Uh oh!

josepjaume Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepjaume commented Mar 21, 2017

Uh oh!

josepjaume commented Mar 21, 2017

Uh oh!

ItsGenis commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrcasals commented Mar 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ItsGenis commented Mar 21, 2017 •

edited

Loading

codecov-io commented Mar 21, 2017 •

edited

Loading

josepjaume Mar 21, 2017 •

edited

Loading