Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat(icon): export icons for compose #607

Closed
wants to merge 6 commits into from
Closed

feat(icon): export icons for compose #607

wants to merge 6 commits into from

Conversation

joheredi
Copy link
Member

@joheredi joheredi commented Dec 13, 2018 •
edited
Loading

This PR exports the following icons in the Teams theme:
image

  • bold
  • code-snippet
  • emoji
  • italic
  • link
  • meeting-new
  • number-list
  • quote
  • sticker
  • table
  • underline

mnajdova
mnajdova reviewed Dec 13, 2018
View reviewed changes
@mnajdova
Copy link
Contributor

Also, please take a look into the screener changed, and ensure that only the new icons are added and there is no regression to the previos icons.

@mnajdova mnajdova closed this Dec 13, 2018
@mnajdova mnajdova reopened this Dec 13, 2018
@kuzhelov
Copy link
Contributor

please, note that you have incorrect target to merge into. Ensure that you have master (not stardust:master) branch for your changes (probably, you will need to create another PR and reapply your changes there):

image

@kuzhelov kuzhelov added the needs author feedback Author's opinion is asked label Dec 13, 2018
johannao76
johannao76 reviewed Dec 13, 2018
View reviewed changes
src/themes/teams/components/Icon/svg/icons/bold.tsx
import * as React from 'react'
import { TeamsProcessedSvgIconSpec } from '../types'

export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this basically a duplicate of the one in ProcessedIcons folder? Why do we need the dupes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm following the template for exporting Icons: #585

@Bugaa92, what's the reason behind duplicating the files?

johannao76
johannao76 approved these changes Dec 13, 2018
View reviewed changes
Copy link
Contributor

@johannao76 johannao76 left a comment

Choose a reason for hiding this comment

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

:shipit:

@joheredi
Copy link
Member Author

please, note that you have incorrect target to merge into. Ensure that you have master (not stardust:master) branch for your changes (probably, you will need to create another PR and reapply your changes there):

image

@kuzhelov I think it shows as stardust-ui:master because I'm merging from my Fork. I'm not an "official" contributor so I couldn't create a branch within the main fork.

@kuzhelov
Copy link
Contributor

@kuzhelov I think it shows as stardust-ui:master because I'm merging from my Fork. I'm not an "official" contributor so I couldn't create a branch within the main fork.

@joheredi, now this omission is fixed :)

@joheredi
Copy link
Member Author

Submitting a new PR against master

@joheredi joheredi closed this Dec 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mnajdova mnajdova mnajdova left review comments

@johannao76 johannao76 johannao76 approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@kuzhelov kuzhelov Awaiting requested review from kuzhelov

@layershifter layershifter Awaiting requested review from layershifter layershifter is a code owner

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny miroslavstastny is a code owner

Assignees
No one assigned
Labels
needs author feedback Author's opinion is asked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joheredi @mnajdova @kuzhelov @johannao76