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

Restructure links to dh.page #566

Conversation

Tguntenaar
Copy link
Collaborator

@Tguntenaar Tguntenaar commented Dec 14, 2023

Resolves #350

All links now direct to devhub.near/widget/dh.${pagename}.

I implented it to be backwards compatible since some users might have bookmarks with the app?page=${pagename} structure in the url.

preview

@Tguntenaar Tguntenaar marked this pull request as ready for review December 15, 2023 10:15
Copy link
Contributor

@elliotBraem elliotBraem left a comment

Choose a reason for hiding this comment

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

@Tguntenaar Is there a reason why in this preview, the Decentralized DevRel Community errors out? The same issue doesn't seem to appear in production.

Otherwise it looks good. Maybe try updating with main, it may be fixed by your "red error flashes" pull request which I just merged

@Tguntenaar
Copy link
Collaborator Author

Tguntenaar commented Dec 18, 2023

@elliotBraem it looks like the same error @Megha-Dev-19 has helped fix today. I will publish a new preview should be fixed already.

@elliotBraem
Copy link
Contributor

elliotBraem commented Dec 18, 2023

Also -- I do want to point out that creating these separate pages does come at a visual cost, albeit a minor one.

You'll see that the current devhub does not reload the entire page when navigating around. This one however, because it does not have a common parent, does reload the header navbar and the rest of the page. I remember we discussed this with @frol's demo here and although it does appear that the header does not rerender (and I was convinced it didn't rerender, too), it in fact does (check inspect element) and it's just because the header is a simple h2 tag that it is not noticeable. Client side routing only works effectively when it operates within a common parent. The VM isn't smart enough (yet) to recognize a common child across renders.

A best-of-both worlds solution may be to keep the pages, but modify the navbar to redirect to the app w/ query param, but this will introduce inconsistency, especially when users want to share a link to the page they are on.

I'll approve this PR, but I'd like second opinion if this is still desirable, considering the cost in user experience @frol @ori-near

Side note:

I want to note that I do think we will find a solution for gateways, pages, and nicer routing that doesn't rely on parameters and satisfies smooth client side routing. I've played around with this idea of a near-bos-webcomponent that takes in a JSON that defines routes and dependencies, which I think would enable us to do something like : devhub.near/widget/app/community/devhub-test. I'll do a spike of this someday soon. And I think there will be improvements on the gateway side, too, potentially with the standardization of the "/app", so something like near.org/devhub.near/widget/app is distilled into near.org/devhub. I'm not suggesting these to prolong this ticket or suggest an alternative, just to weigh priorities and consider possibilities.

@ailisp
Copy link
Collaborator

ailisp commented Dec 20, 2023

Thomas's implementation here looks good overall, but I don't quite understand the benefit of this refactor, @elliotBraem Can you explain a little bit? What is the problem of current approach?

@Tguntenaar
Copy link
Collaborator Author

@ailisp there was a telegram discussion about it, here is the link:
https://t.me/devhubplatform/3/3689

@frol
Copy link
Collaborator

frol commented Dec 21, 2023

Indeed, I feel the UI flashes more than with the current approach, so I feel we are not gaining much with it at this moment :-(

My main motivation was to optimize for search engines (SEO) and avoid the need to have this central "app" component that duplicates BOS routing capabilities, but I think we need to solve it on the BOS VM side first. I recall Next.js had the same problem and their solution is to have special layout.tsx and _app.tsx files.

TL;DR: I think we will have to put this in an icebox and don't merge now.

@Tguntenaar
Copy link
Collaborator Author

I will close this PR now, since it's in the icebox.

@Tguntenaar Tguntenaar closed this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor directory structure for pages
4 participants