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

Replaced financial document icon with new example icon #3828

Merged
merged 6 commits into from
Jun 15, 2020

Conversation

rfultz
Copy link
Contributor

@rfultz rfultz commented Jun 8, 2020

Summary

Impacted areas of the application

Should only affect references to the img/financial-document-circle.svg, which was only in one place in the repo

Screenshots

Icon in Sketch:
image

Related PRs

None

How to test

  • TODO: duplicate the cms db to localhost to test
  • Otherwise, check the Advertising and disclaimers page on dev (linked above)

Next steps

Update the pattern library with this new (replacement) icon

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2020

Codecov Report

Merging #3828 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3828      +/-   ##
===========================================
+ Coverage    75.25%   75.27%   +0.01%     
===========================================
  Files          121      121              
  Lines         7352     7352              
  Branches       592      592              
===========================================
+ Hits          5533     5534       +1     
+ Misses        1819     1818       -1     
Impacted Files Coverage Δ
fec/fec/static/js/modules/calendar.js 92.64% <0.00%> (+0.73%) ⬆️

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 7d956fc...73a8e4d. Read the comment docs.

Copy link
Contributor

@johnnyporkchops johnnyporkchops left a comment

Choose a reason for hiding this comment

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

I think we also need to replace this with an existing or new class for this template option on collection_page.html:

<h2 class="sidebar__title sidebar__title--large icon-heading--financial-document">Reporting examples</h2>

Here is an example of where it is used now:
Screen Shot 2020-06-09 at 7 34 04 AM

@rfultz
Copy link
Contributor Author

rfultz commented Jun 9, 2020

Thanks, @johnnyporkchops—good catch! There's a new version on its way to git now

I think we also need to replace this with an existing or new class for this template option on collection_page.html:

<h2 class="sidebar__title sidebar__title--large icon-heading--financial-document">Reporting examples</h2>

Here is an example of where it is used now:

@johnnyporkchops
Copy link
Contributor

johnnyporkchops commented Jun 10, 2020

@rfultz

  • We should keep the icon-heading--financial-document class and create another for example because we do use it some places.
  • And it maybe good idea to keep the SVG in statsc/img in case we need it later -- finding the svg or recreating it is kind of a pain.

Apologies for not catching/pointing that out in my review comment! Icons on our site can be confusing.

I think @JonellaCulmer would want us to use icon-heading--financial-document primary for middle circle because its a financial thing (?) On home page:
Screen Shot 2020-06-09 at 8 36 58 PM

@JonellaCulmer
Copy link
Contributor

I think @JonellaCulmer would want us to use icon-heading--financial-document primary for middle circle because its a financial thing (?) On home page

@johnnyporkchops You're absolutely right. I didn't even remember that there. My apologies. I thought it would be a clean replacement, but yes, we'll need the financial document to be used there on the homepage.
cc: @rfultz

Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

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

Other than keeping the icon-heading--financial-document mentioned by @johnnyporkchops and @JonellaCulmer, everything else looks good

Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

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

Thanks for restoring the financial document icon

Copy link
Contributor

@johnnyporkchops johnnyporkchops left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@rfultz
Copy link
Contributor Author

rfultz commented Jun 12, 2020

@JonellaCulmer, this is it in the wild:
image

Copy link
Contributor

@JonellaCulmer JonellaCulmer left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @rfultz

@patphongs patphongs merged commit 743c753 into develop Jun 15, 2020
@lbeaufort lbeaufort deleted the feature/3602-icon-for-examples branch June 16, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update example icon globally
5 participants