-
Notifications
You must be signed in to change notification settings - Fork 9
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
Replace GWTQuery with Elemental2 #13
base: main
Are you sure you want to change the base?
Conversation
This commit changes nothing in the output except for how it is produced, and ideally paves the way to more simply producing just the output we need for the website.
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
styleHomepage(); | ||
} | ||
}); | ||
resizeHandler = Window.addResizeHandler(event -> styleHomepage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(far) future cleanup - use elemental here too.
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/highlight/JsHighlight.java
Outdated
Show resolved
Hide resolved
…yPoint.java Co-authored-by: Colin Alworth <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, just some other thoughts here, but take or leave them at your own preference. I'll put up another build of the site on a different domain that uses this branch so we can give it some more testing, then merge?
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
mainTitle = subMenuItem.querySelector("a").textContent + " - " + selectedItem.textContent; | ||
} else { | ||
mainNavigationHref = Window.Location.getPath(); | ||
mainTitle = isOverviewPage(mainNavigationHref) ? "Overview" : "Project"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a change in behavior? I'm not necessarily opposed, but it does make reviewing this giant rewrite a little trickier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since the GQuery
selectors are NPE-safe I had to add the if (selectedItem != null)
check and then it felt weird to replicate the buggy behavior in the else
branch 🤷
This should be the only behavior change though.
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/gwt/site/webapp/client/GWTProjectEntryPoint.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Colin Alworth <[email protected]>
Sounds good, thanks! |
Preview at https://www.gwtproject.org.vertispan.com/ (also includes gwtproject/gwt-site#366 ) |
As discussed in Gitter, GWTQuery breaks navigation when certain Chrome extensions are active.
This should also reduce the code size a bit.
Closes #11 -- all the changes are included here to make Elemental work.
Most functionality stays the same, page title for hompage changed, which makes this conflicted with #1
Fixes #5
Fixes #4