DRYing out icon code #1562
DRYing out icon code #1562
Conversation
This commit just removes duplication for now but does so as a step towards tootsuite/mastodon/1469 also see tootsuite/mastodon/1349 which proposes adding descriptive text to icons (for the toolbar but eventually would be great across the board)
There was a problem hiding this comment.
I realize this quite a lot of work, but I don't see a lot of benefit to doing this, when aria-labels should be added on buttons and links anyway, and not on the icons themselves (right?)
I fear this could introduce a lot of small bugs like the one I commented on.
| @@ -39,7 +40,7 @@ const StatusContent = React.createClass({ | |||
| } else if (link.textContent[0] === '#' || (link.previousSibling && link.previousSibling.textContent && link.previousSibling.textContent[link.previousSibling.textContent.length - 1] === '#')) { | |||
| link.addEventListener('click', this.onHashtagClick.bind(this, link.text), false); | |||
| } else if (media) { | |||
| link.innerHTML = '<i class="fa fa-fw fa-photo"></i>'; | |||
| link.innerHTML = '<Icon icon="photo" />'; | |||
There was a problem hiding this comment.
This won't actually work. Icon is not html, it's a react component. It works with i because that is html.
There was a problem hiding this comment.
@Gargron It works. <Icon> references the icon component just like any of our other components
https://github.com/tootsuite/mastodon/pull/1562/files#diff-38b8991419d1039766d703c68c4a4ebdR3
There was a problem hiding this comment.
sorry @Gargron I stand corrected. I thought that was being parsed by React and assumed you didn't see the new React component. I'll work on fixing that.
There was a problem hiding this comment.
went back to the static HTML string in b18d475
so icon code isn't 100% DRY 😞 due to the one spot .innerHTML is used
I assumed so also but see #1349 (comment)
See #1469 |
@Gargron sorry about this one. It looks like I can’t 100% DRY the icon code out because of innerHTML being used here, so if/when changes are made to icon code need to remember to make them here too
|
@jpdevries thanks for the pull request. It looks like it wasn't something that @Gargron wanted to include in the project, and there are a lot of merge conflicts since opening the pull request. I'm going to recommend we close the pull request, but it's totally open for discussion. |
|
@ashfurrow no problem. if you ever want me to rebase and resubmit it let me know. i think it could be a good step towards a larger theming and accessibility effort and eventually switching to SVG icons #1469 |
|
Yup yup, thanks again! |
…upstream Merge upstream changes
This PR creates a component for icons so that changes to icons in the future can be made in just one place 💯
Just removes duplication for now but does so as a step
towards #1469
also see #1349 which proposes adding descriptive text to icons (for the toolbar but eventually would be great across the board)
Usage
The Icon component just takes the name of the icon now:
You can pass an
activeproperty which will add an.activeclass to the icon.You can also pass a
fixedWidthproperty which will add afa-fwclass to the icon:or override the
iconPrefix:The Icon component sets the
classNamefor you. If for some reason you want to override this, pass aclassNameproperty.