Commit 915a7b46 authored by Robert Knight's avatar Robert Knight

Respond to code review feedback

 - Use the string "blank" instead of `null` as a more explicit indicator
   that a menu item should show a blank space for an icon
 - Rename two variables to indicate their boolean-ness
parent 34b4a571
......@@ -70,7 +70,7 @@ function GroupListItem({
return (
<Fragment>
<MenuItem
icon={group.logo || null}
icon={group.logo || 'blank'}
iconAlt={orgName(group)}
isDisabled={!isSelectable}
isExpanded={isExpanded}
......
......@@ -43,11 +43,11 @@ function MenuItem({
'menu-item__label--submenu': isSubmenuItem,
});
const leftIconSpace = icon !== undefined || isSubmenuItem;
const rightIconSpace = icon !== undefined && isSubmenuItem;
const hasLeftIcon = icon != null || isSubmenuItem; // eslint-disable-line eqeqeq
const hasRightIcon = icon != null && isSubmenuItem; // eslint-disable-line eqeqeq
let renderedIcon = null;
if (icon) {
if (icon !== 'blank') {
renderedIcon = iconIsUrl ? (
<img className={iconClass} alt={iconAlt} src={icon} />
) : (
......@@ -69,7 +69,7 @@ function MenuItem({
role="menuitem"
{...(onClick && onActivate('menuitem', onClick))}
>
{leftIconSpace && (
{hasLeftIcon && (
<div className="menu-item__icon-container">{leftIcon}</div>
)}
{href && (
......@@ -83,7 +83,7 @@ function MenuItem({
</a>
)}
{!href && <span className={labelClass}>{label}</span>}
{rightIconSpace && (
{hasRightIcon && (
<div className="menu-item__icon-container">{rightIcon}</div>
)}
{typeof isSubmenuVisible === 'boolean' && (
......@@ -120,8 +120,8 @@ MenuItem.propTypes = {
* Name or URL of icon to display. If the value is a URL it is displayed
* using an `<img>`, if it is a name it is displayed using `SvgIcon`.
*
* If the property is `null` a blank placeholder is displayed in place of an
* icon. If the property is omitted, no placeholder is displayed.
* If the property is `"blank"` a blank placeholder is displayed in place of an
* icon. If the property is falsey, no placeholder is displayed.
* The placeholder is useful to keep menu item labels aligned in a list if
* some items have icons and others do not.
*/
......
......@@ -42,8 +42,8 @@ describe('MenuItem', () => {
assert.isTrue(wrapper.exists('SvgIcon[name="an-svg-icon"]'));
});
it('renders a blank space for an icon if `icon` is `null`', () => {
const wrapper = createMenuItem({ icon: null });
it('renders a blank space for an icon if `icon` is "blank"', () => {
const wrapper = createMenuItem({ icon: 'blank' });
const iconSpace = wrapper.find('.menu-item__icon-container');
assert.equal(iconSpace.length, 1);
assert.equal(iconSpace.children().length, 0);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment