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 scrolling to anchor with fixed menu #1855

Closed
wants to merge 9 commits into from
Closed

Fix scrolling to anchor with fixed menu #1855

wants to merge 9 commits into from

Conversation

EPolovniy
Copy link

We have bug with scrolling by anchor in headers:

before-scroll-fixed

I propose some fix:

after-scroll-fixed

@reactjs-bot
Copy link

reactjs-bot commented Mar 21, 2019

Deploy preview for reactjs ready!

Built with commit a0a37d2

https://deploy-preview-1855--reactjs.netlify.com

@EPolovniy
Copy link
Author

Hmmm:

yarn run v1.10.1
$ npm-run-all prettier:diff --parallel lint flow
$ yarn nit:source && yarn nit:examples
$ prettier --config .prettierrc --list-different "{gatsby-.js,{flow-typed,plugins,src}/**/.js}"
$ prettier --config examples/.prettierrc --list-different "examples/**/*.js"
$ eslint .
$ flow
No errors!
✨ Done in 7.77s.

@EPolovniy
Copy link
Author

Why anyone don't care on UI\UX fix? :)

@lex111
Copy link
Member

lex111 commented Apr 9, 2019

@alexkrolick please review this.

@EPolovniy
Copy link
Author

@alexkrolick pls help us :)

@lex111
Copy link
Member

lex111 commented Apr 23, 2019

@EPolovniy you can remove unnecessary changes (in content files, *.md)? Better make a new branch, now master is set, you can change it in the current PR.

@lex111
Copy link
Member

lex111 commented Apr 23, 2019

Please revert changes that are not related to this PR.

toPath: version.url,
})),
versions
.filter(version => version.path && version.url)
Copy link
Member

Choose a reason for hiding this comment

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

Is this automatically done? Just change the code style, but why?

Copy link
Author

Choose a reason for hiding this comment

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

This part changes for split function in chain to new line. I think the code looks better now.

@@ -35,33 +32,43 @@ module.exports = (
lastNode.value = lastNode.value.replace(match[1], '');
}

const data = patch(node, 'data', {});
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about changing this. Can we get back the old logic?

Copy link
Author

Choose a reason for hiding this comment

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

This code not necessary and not work :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's just leave these things as they used to be, as well as that new line 🆙. I just want to have changes that solve the current problem.

*.md files and yarn.lock are marked as modified because you created your PR from the master branch. Let's fix this too.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will prepare a new Pull Request

@lex111
Copy link
Member

lex111 commented Apr 24, 2019

@EPolovniy please look this PR #1914
it solves the same problem, but the changes are much smaller, I would accept it better.

Copy link
Author

@EPolovniy EPolovniy left a comment

Choose a reason for hiding this comment

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

in #1914 fix only for h2 and not resolve the problem with anchors

@@ -35,33 +32,43 @@ module.exports = (
lastNode.value = lastNode.value.replace(match[1], '');
}

const data = patch(node, 'data', {});
Copy link
Author

Choose a reason for hiding this comment

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

This code not necessary and not work :)

toPath: version.url,
})),
versions
.filter(version => version.path && version.url)
Copy link
Author

Choose a reason for hiding this comment

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

This part changes for split function in chain to new line. I think the code looks better now.

@EPolovniy
Copy link
Author

*.md files and yarn.lock build automatically

@EPolovniy
Copy link
Author

Add new request #1944

@lex111
Copy link
Member

lex111 commented Apr 24, 2019

Closing in favor of #1944

@lex111 lex111 closed this Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants