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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change icon for Surveys #2433

Merged
merged 2 commits into from Jan 9, 2018
Merged

Change icon for Surveys #2433

merged 2 commits into from Jan 9, 2018

Conversation

seanprashad
Copy link
Contributor

@seanprashad seanprashad commented Dec 29, 2017 •
edited

馃帺 What? Why?

Modified the contents of decidim-surveys/app/assets/images/decidim/surveys/icon.svg to reflect the same .svg found here.

馃搶 Related Issues

馃搵 Subtasks

  • N/A

馃摲 Screenshots (optional)

  • N/A

馃懟 GIF

  • N/A

@seanprashad seanprashad changed the title Fix 2390 - Change icon for Surveys Change icon for Surveys Dec 29, 2017
@codecov
Copy link

codecov bot commented Dec 29, 2017 •
edited

Codecov Report

Merging #2433 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2433      +/-   ##
==========================================
- Coverage   98.68%   98.68%   -0.01%     
==========================================
  Files        1294     1294              
  Lines       30199    30172      -27     
==========================================
- Hits        29802    29775      -27     
  Misses        397      397

@seanprashad
Copy link
Contributor Author

@xabier I've switched over the icon but I wasn't sure whether to keep the original file and change it's contents or delete it and copy of the new file. I went with the former but if I need to change it, please let me know!

@mrcasals
Copy link
Contributor

mrcasals commented Jan 8, 2018

@seanprashad thanks for your PR!

Surveys is a Decidim feature, and it's registered this way:

decidim/decidim-surveys/lib/decidim/surveys/feature.rb

Lines 5 to 9 in 1c39183

Decidim.register_feature(:surveys) do |feature|
feature.engine = Decidim::Surveys::Engine
feature.admin_engine = Decidim::Surveys::AdminEngine
feature.icon = "decidim/surveys/icon.svg"
feature.stylesheet = "decidim/surveys/surveys"

On line 8, we set the path for the icon. This will be used to render menus. If you changed the file name, we would need to change this here.

TLDR: your solution is good, no need to change anything else! 馃槃

mrcasals
mrcasals previously approved these changes Jan 8, 2018
View reviewed changes
@mrcasals mrcasals dismissed their stale review January 8, 2018 08:16

CHANGELOG entry missing

@mrcasals
Copy link
Contributor

mrcasals commented Jan 8, 2018

Oh, now that I check, can you add a CHANGELOG entry for this? It should be under UNRELEASED -> Changed, with a text similar to "Updated icon for surveys feature".

Please copy the style of other entries, updating the link to the PR and so 馃槃

@xabier
Copy link
Contributor

xabier commented Jan 8, 2018

@seanprashad thanks! sorry I was away on hollidays

@xabier
Copy link
Contributor

xabier commented Jan 8, 2018

@mrcasals , give it a merge it all seems completed now! nice job @seanprashad 馃憤

mrcasals
mrcasals approved these changes Jan 9, 2018
View reviewed changes
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Wohoooo! 馃帀 Thanks @seanprashad 馃榿

@mrcasals mrcasals merged commit b0a1d4a into decidim:master Jan 9, 2018
@mrcasals mrcasals mentioned this pull request Jan 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrcasals mrcasals mrcasals approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@seanprashad @mrcasals @xabier