Headings created #855
Headings created #855
Conversation
| import css from './Heading.scss'; | ||
|
|
||
| type Props = { | ||
| large?: boolean, |
There was a problem hiding this comment.
Should all these fields be mandatory rather than optional?
There was a problem hiding this comment.
I think mandatory as then user can use whatever he requires based on props passed. @nshki What are your views on this ?
There was a problem hiding this comment.
I think it makes sense to make the label prop mandatory, but the rest can stay optional.
There was a problem hiding this comment.
I don't know how to make them optional. What do I have to do ?
There was a problem hiding this comment.
You used ? so that should be enough :)
| import css from './Heading.scss'; | ||
|
|
||
| type Props = { | ||
| large?: boolean, |
There was a problem hiding this comment.
I think it makes sense to make the label prop mandatory, but the rest can stay optional.
| return ( | ||
| <span> | ||
| <div className={labelClassNames}>{label}</div> | ||
| </span> |
There was a problem hiding this comment.
Instead of having two layers of elements, let's just keep it at one. I think a heading element would make sense here!
There was a problem hiding this comment.
Which headings should I use. I am thinking
h1 - for large
h2 - for normal
h3- for small
h4- for text
There was a problem hiding this comment.
I personally think we should be using divs that way if we change the hierarchy of headings we don't need to refactor all of H level headings. I think a div without a parent span should be enough!
|
@julianguyen Why are the tests failing ? It is showing Rspec problem. How do I fix it ? |
|
Looks like flakey tests. Argh. I'll look for a way this weekend to reduce the flakiness. Sorry about that! |
|
Okay ! :) |
|
Thanks a lot @julianguyen ! Your kind words keep me and Prateksha motivated ❤️ I'll merge it after the tests ? |
|
Yep yep! It should go green 🍀 |
|
@julianguyen Could you trigger it again ? Tests failed. I do not have write permissions and hence cant trigger it ! |
|
Looks like there are legitimate issues this time around. Scroll down the page to where the red box is highlighted for more details! Let me know if you need help. I'll be away from internet for an hour or so though. |
|
@julianguyen Can you please approve again ? The tests finally passed :) |
issue #846 resolved.
Created Headings .