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

#66: Should work without CSS in modern browsers (closes #66) #68

Merged
merged 11 commits into from
Jul 2, 2018

Conversation

FrancescoCioria
Copy link
Contributor

@FrancescoCioria FrancescoCioria commented Jun 29, 2018

Closes #66

Test Plan

tests performed

  1. works on Chrome
  2. works on IE11 on Windows 8.1

tests not performed (domain coverage)

At times not everything can be tested, and writing what hasn't been tested is just as important as writing what has been tested.

An example of partial test is a field displaying 4 possible values. If 3 values are tested, with screenshots, and 1 is not, then it should be mentioned here.

Copy link
Member

@giogonzo giogonzo left a comment

Choose a reason for hiding this comment

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

Looks 💯!

(unrelated to this pr) Are any of the "known issues" with IE11 here https://caniuse.com/#feat=flexbox worth a warning?

src/FlexView.tsx Outdated
justifyContent: alignPropToFlex(column ? vAlignContent : hAlignContent),
alignItems: alignPropToFlex(column ? hAlignContent : vAlignContent),

// style passed throw props
Copy link
Member

Choose a reason for hiding this comment

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

through?

src/FlexView.tsx Outdated
const style = this.getStyle();
const props = omit(this.props, Object.keys(FlexView.propTypes));
return (
<div className={className} style={style} {...props}>
<div className={`react-flex-view ${this.props.className || ''}`.trim()} style={style} {...props}>
Copy link
Member

Choose a reason for hiding this comment

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

Are you leaving the class to be slightly less breaking or some other reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think we are relying on that class in some projects... for modern browsers this PR should not be breaking (apart from removing the import of the no longer existing SASS)

@FrancescoCioria FrancescoCioria merged commit 0eb99d9 into master Jul 2, 2018
@nemobot
Copy link
Contributor

nemobot commented Jul 2, 2018

@nemobot nemobot removed the in review label Jul 2, 2018
@FrancescoCioria FrancescoCioria deleted the 66-should_work_without_css branch July 2, 2018 13:48
Sign up for free to 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.

3 participants