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

Get it working with the imported icons. #59

Merged
merged 4 commits into from
May 18, 2018
Merged

Conversation

bwinton
Copy link
Collaborator

@bwinton bwinton commented Feb 9, 2018

Hey Tim and Erica,

we're starting to export some icons from the photon design system, and ideally eventually use those to generate this site. How does this code look to you? Understandable? Any changes you'd recommend?

@aminalhazwani
Copy link
Collaborator

@ericawright @nt1m do you folks have any feedback? Thank you very much 🙇 For more context see https://github.com/FirefoxUX/design-tokens/tree/master/photon-icons

@nt1m
Copy link
Collaborator

nt1m commented Feb 22, 2018

The general approach looks ok to me, I'm just wondering if we can't simplify this further by not having a deployment script and simply fetching the icons+listing directly from npm.

@bwinton bwinton changed the title WIP: Get it working with the imported icons. - Do Not Merge Yet! Get it working with the imported icons. May 15, 2018
@bwinton
Copy link
Collaborator Author

bwinton commented May 15, 2018

Okay, I think this is ready for the actual review. Have at it, everyone! 😃

(@nt1m I think I agree that we can use the icons+listing directly from npm. But I'd like to make that a followup bug, if you don't mind.)

@bwinton
Copy link
Collaborator Author

bwinton commented May 15, 2018

screenshot

Copy link
Collaborator

@aminalhazwani aminalhazwani left a comment

Choose a reason for hiding this comment

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

  • $ npm run dev prompts an error
  • once we merge Make 'deprecated' a tag instead of a category. photon-icons#1 we can problably bump the npm version to v3.1.0
  • we can remove the /viewer subfolder, feel free to open a following issue edit: we have a issue for the /viewer see Remove /viewer container as it is not needed anymore #43
  • what is the difference between /dist and /html
  • there's an empty folder /icons/extensions
  • we can remove most of the contents from the README.md
  • with $ npm run watch, if I select an icon in the grid list, the sidebar opens, but the icon is not displayed in the preview

Icons Screenshot

@bwinton
Copy link
Collaborator Author

bwinton commented May 18, 2018

  • $ npm run dev prompts an error

No it doesn't. 😉 (It's impossible for me to do anything with this unless you paste the error you're seeing…)

Done.

  • we can remove the /viewer subfolder, feel free to open a following issue

I'll let someone else open that issue, if they feel strongly.

  • what is the difference between /dist and /html

/dist is where the generated files go. /html is the source.

  • there's an empty folder /icons/extensions

Not anymore. 😃

  • we can remove most of the contents from the README.md

I think it's still kinda useful, so I don't really want to do that.

  • with $ npm run watch, if I select an icon in the grid list, the sidebar opens, but the icon is not displayed in the preview

That would depend on how you're starting the server, I suppose. Maybe try the new npm run dev and see if it's better, or if not, paste the location of the missing image here so I can see why it's missing…

@aminalhazwani
Copy link
Collaborator

No it doesn't. 😉 (It's impossible for me to do anything with this unless you paste the error you're seeing…)

It works, I was kidding!

I'll let someone else open that issue, if they feel strongly.

I edited the comment above, we already had an issue, see #43

I think it's still kinda useful, so I don't really want to do that.

I agree that is useful, but this is not the place where this information is most useful. As you said here we should have info and request specifically to the website, not the icons them self, let me open an issue to address this if it sounds good to you!

That would depend on how you're starting the server, I suppose. Maybe try the new npm run dev and see if it's better, or if not, paste the location of the missing image here so I can see why it's missing…

Yes, thanks Blake!

@aminalhazwani aminalhazwani merged commit 24c752e into master May 18, 2018
@bwinton bwinton deleted the import-icons branch May 18, 2018 15:00
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