Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add no-skin lasso flag #84

Merged
merged 4 commits into from
Apr 9, 2018
Merged

Conversation

CestDiego
Copy link
Contributor

Description

Add condition to now load skin with each component if there is a flag present.

Context

Global Header ships their own scoped skin version and using ebayui-core components ships with their own unscoped skin code, this defeats the purpose of having a scoped skin version. This PR fixes that problem by letting Global Header import components without skin, and injecting their own scoped skin into then.

References

#75

@CestDiego CestDiego changed the base branch from master to 0.2.0 April 6, 2018 15:17
@seangates
Copy link
Contributor

I worked with @CestDiego on this PR this morning. It’s legit.

Thanks for jumping on this, Diego!

@coveralls
Copy link
Collaborator

coveralls commented Apr 6, 2018

Pull Request Test Coverage Report for Build 285

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.636%

Totals Coverage Status
Change from base Build 273: 0.0%
Covered Lines: 430
Relevant Lines: 472

💛 - Coveralls

@ianmcburnie ianmcburnie self-requested a review April 6, 2018 15:57
@senthilp
Copy link

senthilp commented Apr 6, 2018

Thanks @CestDiego and @seangates

Copy link
Contributor

@seangates seangates left a comment

Choose a reason for hiding this comment

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

Things look great to me, but I want others to respond in the affirmative before we merge.

@yomed
Copy link
Contributor

yomed commented Apr 6, 2018

I know the name was discussed a bit earlier, but skinless seems a little weird because skin and less are both terms in this CSS space. There's even skin/less.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Apr 6, 2018

I agree with Yoni, skinless is cute (if your name is Hannibal Lecter), but ripe for confusion. Maybe ebayui-unstyled or ebayui-nostyle or something along those lines...

@seangates
Copy link
Contributor

seangates commented Apr 6, 2018

Proposed names:

ebayui-nostyle
ebayui-nostyles
ebayui-no-styles
ebayui-noskin
ebayui-no-skin
ebayui-unstyled

@yomed
Copy link
Contributor

yomed commented Apr 6, 2018

Personally, I think no-skin is the clearest and most accurate in terms of what we're actually doing. I imagine all the component users know what Skin is, so that shouldn't be an issue.

Are we intentionally leaving this out of the documentation?

@seangates
Copy link
Contributor

I expect we leave this out of the documentation. It is only pertinent in very specific cases. In fact, only one we know of.

@ianmcburnie
Copy link
Contributor

Let's go with ebayui-no-skin, undocumented. Nobody will know about this feature except us and the people who are lurking in this thread (I'll create an issue to have their memories erased).

@CestDiego
Copy link
Contributor Author

@ianmcburnie sorry for the delay. I have changed the skinless flag to the no-skin alternative.

@seangates
Copy link
Contributor

seangates commented Apr 7, 2018

Build failed, this time on something in the carousel. @yomed, can you look at it on Monday?

@seangates
Copy link
Contributor

Never mind. Reran the build. It was simply a timeout.

@yomed yomed changed the title Add skinless flag solves #75 Add no-skin lasso flag Apr 9, 2018
@yomed
Copy link
Contributor

yomed commented Apr 9, 2018

@CestDiego Would ebay-breadcrumb need this too?

"@ebay/skin/breadcrumb",
"@ebay/skin/utility",

@CestDiego
Copy link
Contributor Author

@yomed We definitely need that. I branched off of master when making changes then cherry picked on top of 0.2.0 and I missed this. Latest commit 799038d fixes this

@seangates seangates merged commit 04bc4ce into eBay:0.2.0 Apr 9, 2018
@CestDiego CestDiego deleted the add-skinless-flag branch April 10, 2018 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants