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

fix(exoflex): fix unable to change inner bar height #159

Merged
merged 6 commits into from
Nov 1, 2019

Conversation

nathaniaelvina
Copy link
Contributor

  • change some styling so it's more similar to RNP
  • enable the user to change inner bar height
    Simulator Screen Shot - iPhone 5s - 2019-10-29 at 12 03 56

@nathaniaelvina nathaniaelvina added wip Work in progress ready for review and removed wip Work in progress labels Oct 29, 2019
oshimayoan
oshimayoan previously approved these changes Oct 29, 2019
Copy link
Contributor

@oshimayoan oshimayoan left a comment

Choose a reason for hiding this comment

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

Looks great!

let borderRadius = (style && style.borderRadius) || roundness;

let onLayout = (event: LayoutChangeEvent) =>
setWidth(event.nativeEvent.layout.width);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just use flex: 1 or width: '100%' instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I just mimic how paper implemented it. we can change back to width: '100%'

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think I get what Paper wants to by using onLayout. Could you please check it on a browser and resize the browser width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using width: '100%'
Kapture 2019-10-30 at 11 39 18

using onLayout
Kapture 2019-10-30 at 11 36 13

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought width: '100%' won't work well on desktop browser. But actually, it works better than onLayout.

Comment on lines 23 to 24
let height = (style && style.height) || 8;
let borderRadius = (style && style.borderRadius) || roundness;
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't access style object directly.
Either we flatten it first or use a different approach.

Quiz time! What happen if the user pass in something like style={[styles.foo, { height: 7 }]}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure we can flatten it.

On that case, the height will be 7 but the radius stay the same, unless they pass the new radius
Screen Shot 2019-10-30 at 11 45 41 AM

Copy link
Member

Choose a reason for hiding this comment

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

Right, my point is in the style.borderRadius part. We're not supposed to access it directly.

On older version of RN, the style prop could be an array or an object, or even a number!

Can you guess where the number came from? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

righttt, the StyleSheet.create returns number

darcien
darcien previously approved these changes Nov 1, 2019
Copy link
Member

@darcien darcien left a comment

Choose a reason for hiding this comment

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

🐑 it

@@ -20,7 +20,10 @@ export default function ProgressBar(props: Props) {
duration: 500,
});

let height = (style && style.height) || 8;
let flattenedStyle = StyleSheet.flatten<ViewStyle>(style);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let flattenedStyle = StyleSheet.flatten<ViewStyle>(style);
let flattenedStyle = StyleSheet.flatten<ViewStyle>(style) || {};

So we can drop the flattenedStyle &&

@oshimayoan oshimayoan added this to To do in exoflex v2.7 Nov 1, 2019
@oshimayoan oshimayoan added this to the exoflex v2.7.0 milestone Nov 1, 2019
@nathaniaelvina nathaniaelvina merged commit 0352647 into kodefox:master Nov 1, 2019
oshimayoan pushed a commit that referenced this pull request Nov 6, 2019
* style(exoflex): change progress bar styling

* docs(exoflex): update example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
exoflex v2.7
  
To do
Development

Successfully merging this pull request may close these issues.

None yet

3 participants