Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Conversation

@alongoni
Copy link
Contributor

@alongoni alongoni commented Mar 30, 2021

Related to #109.

Motivation

Every time we are using an icon + label in SR (safe-react) or SRC (safe-react-components) we need to add a css wrapper in order to align the text and the icon.
This component intends to fix that problem.

What's new

Its adds two new properties to IconText, side (left or right) and margin.

  • side: used to indicate at what side the icon will be rendered. In SR, we use both icon + text for simple labels and text + icon for links.
  • margin: add some space between the icon and text.

How to test it

As this repo only provides components and as we making use of storybook, it would be enough to have a story that represents all the variants for each component. In the PR description, you will find a link to a deployed instance of storybook. For this particular PR the link is https://pr113--safereactcomponents.review.gnosisdev.com.

cc: @katspaugh

@github-actions
Copy link

github-actions bot commented Mar 30, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Mar 30, 2021

Travis automatic deployment:
https://pr113--safereactcomponents.review.gnosisdev.com

@alongoni alongoni self-assigned this Mar 30, 2021
@ghost
Copy link

ghost commented Mar 30, 2021

Travis automatic deployment:
https://pr113--safereactcomponents.review.gnosisdev.com

@alongoni alongoni marked this pull request as ready for review April 5, 2021 17:27
@alongoni alongoni requested a review from nicosampler April 5, 2021 17:28
@nicosampler nicosampler changed the title IconText props feature: Add margin and side props to IconText Apr 8, 2021
@nicosampler nicosampler requested review from dasanra and katspaugh April 8, 2021 15:49
Copy link
Contributor

@nicosampler nicosampler left a comment

Choose a reason for hiding this comment

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

There is a problem with the margins
image

@katspaugh
Copy link
Member

@alongoni as per the retro discussion, could you add a short summary in the PR description?
What has been done, and how to test it (which icons in the UI were affected specifically).

@nicosampler nicosampler requested a review from fernandomg April 9, 2021 11:38
@katspaugh
Copy link
Member

Shouldn't margin automatically appear in the list of props in the storybook?
I can see iconSide in the list but not margin. 🤔

@nicosampler
Copy link
Contributor

nicosampler commented Apr 9, 2021

@katspaugh seems like a problem with the last build. Running it locally works as expected.

image

@nicosampler
Copy link
Contributor

@alongoni you can run this command yarn tsc to see all the places that are broken

@ghost
Copy link

ghost commented Apr 9, 2021

Travis automatic deployment:
https://pr113--safereactcomponents.review.gnosisdev.com

@alongoni alongoni requested a review from nicosampler April 9, 2021 17:25
@ghost
Copy link

ghost commented Apr 9, 2021

Travis automatic deployment:
https://pr113--safereactcomponents.review.gnosisdev.com

@katspaugh
Copy link
Member

katspaugh commented Apr 10, 2021

Looks good!

Maybe we could make the default margin value depend on the text size? If the text is small, the margin should be also small. And vice versa. Then you wouldn't have to pass it every time. Never mind, 6px looks nice with all the sizes.

@alongoni alongoni merged commit aef5df2 into development Apr 14, 2021
@alongoni alongoni deleted the issue#109/IconText branch April 14, 2021 19:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants