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.

Sign up for GitHub

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

Jump to bottom

[NCL-7777] Fix Content jumping issue #239

Merged
merged 1 commit into from
Jul 11, 2023
Merged

[NCL-7777] Fix Content jumping issue #239

merged 1 commit into from
Jul 11, 2023

Conversation

DnsZhou
Copy link
Contributor

@DnsZhou DnsZhou commented Jun 24, 2023

No description provided.

@DnsZhou
Copy link
Contributor Author

DnsZhou commented Jun 24, 2023

See jira NCL-7777 for gifs

matedo1
matedo1 reviewed Jun 26, 2023
View reviewed changes
Copy link
Contributor

@matedo1 matedo1 left a comment

Choose a reason for hiding this comment

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

See jira comments

@DnsZhou DnsZhou requested a review from matedo1 June 30, 2023 01:17
@DnsZhou
Copy link
Contributor Author

DnsZhou commented Jun 30, 2023 •
edited
Loading

H @matedo1 , changes applied and see jira for the latest gifs.

@matedo1
Copy link
Contributor

matedo1 commented Jul 7, 2023

Hi @patrikk0123 , could you please finish code review for this one?

@DnsZhou could you please squas your commits into one or two? Nine commits seem to be a little bit confusing in this case as finally just a few simply lines were modified.

@matedo1 matedo1 removed their request for review July 7, 2023 08:49
patrikk0123
patrikk0123 reviewed Jul 10, 2023
View reviewed changes
Copy link
Contributor

@patrikk0123 patrikk0123 left a comment

Choose a reason for hiding this comment

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

Some code comments.

src/components/Tabs/TabsItem.tsx Outdated Show resolved Hide resolved
src/components/Tabs/Tabs.tsx Outdated Show resolved Hide resolved
src/components/ProductMilestonePages/ProductMilestonePages.tsx Outdated Show resolved Hide resolved
@DnsZhou
Copy link
Contributor Author

DnsZhou commented Jul 10, 2023

I have force-pushed the changes into one commit and extracted the component. See jira NCL-7777 for gifs that fixed the error.

matedo1
matedo1 requested changes Jul 10, 2023
View reviewed changes
src/components/ProductMilestonePages/ProductMilestonePages.tsx Outdated Show resolved Hide resolved
src/components/Tabs/Tabs.module.css Outdated Show resolved Hide resolved
src/components/Tabs/TabsLabel.tsx Outdated Show resolved Hide resolved
@DnsZhou DnsZhou requested a review from matedo1 July 10, 2023 18:38
matedo1
matedo1 requested changes Jul 11, 2023
View reviewed changes
src/components/Tabs/TabsLabel.tsx Outdated Show resolved Hide resolved
@DnsZhou DnsZhou requested a review from matedo1 July 11, 2023 12:25
patrikk0123
patrikk0123 approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@patrikk0123 patrikk0123 left a comment

Choose a reason for hiding this comment

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

I approve, just please squash commits.

@DnsZhou
Copy link
Contributor Author

DnsZhou commented Jul 11, 2023

Hi @patrikk0123 , there are only 3 commits now, it was already squashed in the last force push. wold that be okay?

@patrikk0123
Copy link
Contributor

Hi @patrikk0123 , there are only 3 commits now, it was already squashed in the last force push. wold that be okay?

Count does not matter that much, but commits contradict themselves - add tooltip; bring tooltip back.
The final branch before merging should really be divided based on content and historical changes as the branch was developed need to be hidden by squashing.

@DnsZhou
Copy link
Contributor Author

DnsZhou commented Jul 11, 2023

squashed and merged. Thanks @patrikk0123

@DnsZhou DnsZhou merged commit 7968dcc into project-ncl:main Jul 11, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrikk0123 patrikk0123 patrikk0123 approved these changes

@matedo1 matedo1 Awaiting requested review from matedo1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DnsZhou @matedo1 @patrikk0123