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

implement lexicographic navigation #68

Merged
merged 4 commits into from
Jul 17, 2023
Merged

implement lexicographic navigation #68

merged 4 commits into from
Jul 17, 2023

Conversation

OrkWard
Copy link
Contributor

@OrkWard OrkWard commented Jun 23, 2023

@RalXYZ
Copy link
Member

RalXYZ commented Jun 23, 2023

6,牛逼

@OrkWard
Copy link
Contributor Author

OrkWard commented Jun 23, 2023

虽然 index.jsx 看起来改了很多其实只是不小心缩进的原因导致(
我应该只在最后面添加了一个新的 component 然后把整个用 <></> 包起来(毕竟不能直接 return 多个并列标签)

@RalXYZ
Copy link
Member

RalXYZ commented Jun 23, 2023

@OrkWard I have reviewed this pull request and completed all the comments. If you have better suggestions regarding the comments, we can continue the discussion.

@RalXYZ RalXYZ added the enhancement New feature or request label Jun 23, 2023
@RalXYZ RalXYZ self-assigned this Jun 23, 2023
@RalXYZ RalXYZ linked an issue Jun 23, 2023 that may be closed by this pull request
@OrkWard
Copy link
Contributor Author

OrkWard commented Jun 23, 2023

I think a better method to perform navigation is using window.srollTo or Element.prototype.scrollIntoView instead of hashtag, which causes no change in window.location, allow history navigation working properly; and may improve performance(I doubt change hash will cause some rerender in React)
will try this in spare time, if work as expected I'll close this and open a new pr

@wfwf1997
Copy link

wfwf1997 commented Jun 24, 2023

I suppose hash navigation is totally fine. It's simple but works. Actually, it is a quite common practice to use hash for quick navigation in a long articles. The url with hash can also make people who open the link jump to the target paragraph faster. React will not trigger re-rendering when the hash changes by default. And if you're really concerned with the history, just use history.replaceState. But for me, I don't mind the change.

@wfwf1997
Copy link

@RalXYZ I suppose we could also add hash for each mirror card!

@OrkWard
Copy link
Contributor Author

OrkWard commented Jun 24, 2023

Glad to heard that, I don't mean to pursue perfect either, just merge it if no other problem

Also it won't be busy in my 4th year, so if need I can participate in maintainance

@RalXYZ
Copy link
Member

RalXYZ commented Jun 27, 2023

Also it won't be busy in my 4th year, so if need I can participate in maintainance

Thank you very much! We really appreciate your contribution, but we don't want to burden you with too much work. After all, contributing to open-source communities may not bring you material benefits or provide significant technical and product experience compared to participating in internships. Furthermore, our current version of the frontend is already approaching perfection, and there are few new features to be added.

In the future, if we have new requirements that need to be implemented, we will create an issue for it. This information will not be directly notified to you, as we don't want it to be like assigning tasks. If you are willing to implement a specific issue, you can communicate with us there.

Contributions are welcomed 😊

@OrkWard
Copy link
Contributor Author

OrkWard commented Jun 27, 2023

Thank you for your patient reply. However contribution is more like a kind of fun and learning experience...in that case I'll participate in my own pace

@iamNCJ iamNCJ merged commit 8e2f6f5 into ZJUSCT:main Jul 17, 2023
@iamNCJ
Copy link
Contributor

iamNCJ commented Jul 17, 2023

Really thank you for your great work! @OrkWard

@fish98
Copy link
Member

fish98 commented Jul 17, 2023

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI/UX] Improve index page display
5 participants