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

Optimize icons #943

Merged
merged 5 commits into from
Sep 2, 2019
Merged

Optimize icons #943

merged 5 commits into from
Sep 2, 2019

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Aug 30, 2019

Also moves most of these to live in the root assets/ directory, but no strong feelings about keeping them here vs. locally.

@auto-assign auto-assign bot requested review from AquiGorka and bpierre August 30, 2019 22:10
Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

Also moves most of these to live in the root assets/ directory, but no strong feelings about keeping them here vs. locally.

I generally keep them local when they are only used by a given component (in a local assets/) so that the component stays self contained. But for the illustrations, I agree it might make more sense to keep them all together. 👍

import { useHelpScout } from '../../HelpScoutBeacon/useHelpScout'
import helpAndFeedbackPng from '../../../assets/help-and-feedback.png'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say this one makes more sense locally, just like Pierre's comment, I try to keep assets locally for component's to be self-contained, when one asset is used more than once (only then) I move them to a global point of import.

@@ -21,7 +21,7 @@ import {
import useBeaconSuggestions from './useBeaconSuggestions'
import { useHelpScout } from './useHelpScout'
import BeaconHeadScripts from './BeaconHeadScripts'
import headerImg from './header.png'
import helpScoutHeaderPng from '../../assets/help-scout-header.png'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as other component.

Copy link
Contributor

@AquiGorka AquiGorka 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 should consider this on a per-item basis, some assets make sense at top level while other which are not used else where should be kept locally.

@vercel vercel bot temporarily deployed to staging September 2, 2019 08:03 Inactive
@sohkai
Copy link
Contributor Author

sohkai commented Sep 2, 2019

I've moved the mentioned assets back into their local directories.

I've opted to keep no-results shared, since this seems to be a fairly generic assets that could be made available through aragonUI 🤷‍♂

@sohkai sohkai merged commit 5d0036e into newstyle Sep 2, 2019
@sohkai sohkai deleted the optimize-icons branch September 2, 2019 08:05
2color added a commit that referenced this pull request Sep 2, 2019
…tions

* origin/newstyle:
  Optimize icons (#943)
  PropTypes: move fetch-required fields of AppType to be non-requi… (#946)
  Update ENS API changes from @aragon/wrapper (#944)
  Permissions: fix debounced search state on clearing filters (#942)
  Updates for aragonUI and other clean up (#939)
  AppCenter: update copy (#940)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants