Skip to content
This repository was archived by the owner on Mar 13, 2025. It is now read-only.

fix: issue#54 #59

Merged
merged 1 commit into from
Jan 19, 2021
Merged

fix: issue#54 #59

merged 1 commit into from
Jan 19, 2021

Conversation

yoution
Copy link
Contributor

@yoution yoution commented Jan 19, 2021

No description provided.

@yoution
Copy link
Contributor Author

yoution commented Jan 19, 2021

#54

@yoution yoution requested a review from maxceem January 19, 2021 10:27
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@yoution works great, just one code improvement below:

@maxceem
Copy link
Contributor

maxceem commented Jan 19, 2021

@yoution one more thing. We use pagination not only on the teams list, but also on other pages. After this fix, the vertical alignment is broken on other pages (there is extra space under the page number now):

image

See http://localhost:8080/taas/myteams/16882

@yoution yoution requested a review from maxceem January 19, 2021 12:41
Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@yoution there are still 2 things:

@@ -15,6 +17,9 @@
border-color: #2A2A2A;
}

> button[class] {
Copy link
Contributor

Choose a reason for hiding this comment

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

@yoution this technically works, but if someone new look at this it's not clear what does it mean and why we did so. Could we replace it with some more clear way, for example,.page, .next, .prev { margin-bottom: 10px; }, it's more wordy, but it's more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxceem ok , I will add new classname for the button
but how about #59(comment), it still not work?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yoution this #59 (comment) already works, thanks!

@maxceem
Copy link
Contributor

maxceem commented Jan 19, 2021

Sorry @yoution this is fixed #59 (comment)

So only this one is left #59 (comment)

@yoution yoution requested a review from maxceem January 19, 2021 13:22
@maxceem
Copy link
Contributor

maxceem commented Jan 19, 2021

Example link to comment #59 (comment)

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@yoution after the last fix when I run npm run dev I get error:

image

@yoution yoution requested a review from maxceem January 19, 2021 13:37
@yoution
Copy link
Contributor Author

yoution commented Jan 19, 2021

@maxceem sorry, push again

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Thanks for all the small fixes @yoution. Works perfect now.

@maxceem maxceem merged commit c0de69d into topcoder-archive:dev Jan 19, 2021
@yoution yoution deleted the hotfix/issue-54 branch June 22, 2021 09:41
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