Skip to content
This repository was archived by the owner on Apr 23, 2020. It is now read-only.

added colors for circle and triangle as well as trianglePosition - #3

Open
Zefau wants to merge 13 commits into
ftes:masterfrom
Zefau:master
Open

added colors for circle and triangle as well as trianglePosition#3
Zefau wants to merge 13 commits into
ftes:masterfrom
Zefau:master

Conversation

@Zefau

@Zefau Zefau commented May 4, 2019

Copy link
Copy Markdown

Hi there,

thanks for your work.

I added the possibilities to have different colors on circles, lines and triangle. Additionally, I added an option to have the triangle either on top of the box or the bottom of it.
Furthermore, I added an offset for the pop-in effect of the boxes to be delayed.

Cheers,
Zefau

@ftes

ftes commented May 6, 2019

Copy link
Copy Markdown
Owner

Thank you for the PR!
I am currently traveling and won't be able to review in the next month.

Do you have any timeline when you want or need an updated version?

@Zefau

Zefau commented May 6, 2019

Copy link
Copy Markdown
Author

Hi ftes. Thanks for the quick reply. The pr is not in rush.
Enjoy travelling! Cheers

@ftes ftes left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

When running the example, I see broken inconsistent placing of even and odd (left/right) elements. Can you reproduce this?

image

Comment thread README.md
color | black
twoSidedOverlap | 80 | negative overlap between items if two-sided
animations | true
animationScrollDelay | 100 | Delays triggering the animation when scrolling up or down

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The wording leaves it unclear if this a time-based or offset/space-based delay.

From the code I see that this is an offset delay of 100px, I think it should

  1. be mentioned in the description
  2. possibly added to the variable name.

Comment thread src/content.js
const rect = this.children.getBoundingClientRect()
const vwHeight = window.innerHeight || document.documentElement.clientHeight
let vwHeight = window.innerHeight || document.documentElement.clientHeight
let viewTop = window.pageYOffset > vwHeight ? vwHeight : vwHeight - this.props.config.animationScrollDelay

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Indentation inconsistent. Should add auto code formatting check for PRs.

Comment thread src/content.js
+ evenItemOffset
const offsetEvenNml = offsetEven + itemWidth
const offsetEvenMed = offsetEven + itemWidthMed
const offsetOddNml = circleWidth / 2 + paddingToItem + triangleWidth;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Indentation.

Comment thread src/entry.js
base: {
position: 'absolute',
bottom: '0',
top: `${

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Indentation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants