Skip to content

added Glamour and remove Radium in Listbox - #757

Closed
huyqdang wants to merge 3 commits into
masterfrom
hd/rad_to_glam_listbox
Closed

added Glamour and remove Radium in Listbox#757
huyqdang wants to merge 3 commits into
masterfrom
hd/rad_to_glam_listbox

Conversation

@huyqdang

Copy link
Copy Markdown
Contributor

it seems like this is all I have to do, but is it ? Am I missing anything ?

@huyqdang
huyqdang requested a review from paniclaterJuly 11, 2018 20:37
@paniclater

Copy link
Copy Markdown
Collaborator

I believe that this is all you need to do in most components - however there may be some trickier issues around the media queries for breakpoints that may pop up. Most components should be similar to this though.

@paniclater

Copy link
Copy Markdown
Collaborator

@phantomxc@cerinman@sambev am I correct here?

@paniclater

Copy link
Copy Markdown
Collaborator

sorry accidentally clicked close 😳

@sambev

Copy link
Copy Markdown
Contributor

I think this is the general idea, but we do want to be taking a close look at differences, sometimes it isn't always a pure 1:1.

@sambev

Copy link
Copy Markdown
Contributor

Actually I think you need to change it from style to className

@huyqdang

Copy link
Copy Markdown
ContributorAuthor

@sambev Oh my god, thanks Sam. I was stuck for 30 minutes wondering why the styles are jacked up. 🤦‍♂️

@cerinman

Copy link
Copy Markdown
Contributor

Yes, 1:1 for most cases but as an example, the Drawer component uses break points so we'll have to make sure that is covered.

I would also suggest looking for merged styles. In the case of radium, there where places where we used the radium syntax that will have to be updated to object spread.

Radium syntax

<divstyle={[styles.item,isSelected&&styles.selectedItem]}>

Object spread

<divstyle={{ ...styles.item, ...isSelected ? styles.selectedItem : {}}}

<div
aria-label={this.props['aria-label']}
className='mx-listbox'
className={'mx-listbox' + css({ ...this.props.style })}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You shouldn't have to spread this.props.style in a new object here. You should be able to just do.

className={'mx-listbox'+css(this.props.style)}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might even be able to use string interpolation as well

className={`mx-listbox ${css(this.props.style)}`}

@cerinman

Copy link
Copy Markdown
Contributor

Closing this in favor of pursuing a style sheet for components.

@cerinman
cerinman deleted the hd/rad_to_glam_listbox branch November 8, 2018 18:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@huyqdang@paniclater@sambev@cerinman