-
-
Notifications
You must be signed in to change notification settings - Fork 800
Upgrade UI #818
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
Upgrade UI #818
Conversation
Co-authored-by: Matthias Fechner <matthias@fechner.net>
Co-authored-by: Matthias Fechner <matthias@fechner.net>
Co-authored-by: Matthias Fechner <matthias@fechner.net>
Adds 2mb artifact size for syntax highlighting
reduces main js size by 400kb
| endif | ||
| DOCKER_BUILD_IMAGE=gotify/build | ||
| DOCKER_WORKDIR=/proj | ||
| DOCKER_RUN=docker run --rm -e LD_FLAGS="$$LD_FLAGS" -v "$$PWD/.:${DOCKER_WORKDIR}" -v "`go env GOPATH`/pkg/mod/.:/go/pkg/mod:ro" -w ${DOCKER_WORKDIR} |
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.
@eternal-flame-AD Do you have time to give this PR a small manual test? It should work as the e2e tests are successful, but there may be something I overlooked.
The UI coloring changed a little because the defaults of the material-ui lib changed.
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.
Sure, will do today or tomorrow!
Previously, expanded messages where collapsed again when scrolling some messages further. The saved height in the virtual list wasn't changed. This caused the list to flicker when scrolling to the previously expanded message again.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #818 +/- ##
==========================================
- Coverage 79.55% 79.54% -0.01%
==========================================
Files 56 56
Lines 2646 2645 -1
==========================================
- Hits 2105 2104 -1
Misses 450 450
Partials 91 91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
eternal-flame-AD
left a comment
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.
Maybe we can try some service worker stuff so latest messages always displays regardless of Internet connection, similar to the Android App.
|
Here's a demo script that creates a variety of messages under realistic scenarios (and they are all free assets so we can use it for branding). It would also be nice if we can get clickURL to work on web frontend. https://gist.github.com/eternal-flame-AD/de79b3431281c802548d9c0b4dbbdc47
|
Signed-off-by: eternal-flame-AD <yume@yumechi.jp>
fix message layout
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.
lgtm, I don't see any regressions so good to merge. It's a long wanted feature so I am good with just get it out of the door first and then we can see maybe new features (the service worker and more interactive extras support)
Maybe update the screenshot because the UI now looks different.
| SHELL := /bin/bash | ||
| ifdef GOTOOLCHAIN | ||
| GO_VERSION=$(GOTOOLCHAIN) | ||
| else |
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.
Reconnecting doesn't correctly refresh the currently visible data.
reconnect.webm
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.
Not sure how you made this happen, I blocked /message and clicked refresh and can get stuck in this state.
Probably it's MessagesStore#loadMore needs a .finally(() => (this.loading = false)), I think it is an old bug bc IIRC I accidentally got it stuck refreshing before too.
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 is a separate problem, I'll fix this too. The one I can reproduce happens after restarting gotify/server. The cause of this bug is that the state is cleared after a reconnect happens (there could be new messages while the websocket was disconnected, so we do a full refresh). This happens here
Lines 37 to 45 in 43574a0
| reaction( | |
| () => stores.currentUser.connectionErrorMessage, | |
| (connectionErrorMessage) => { | |
| if (!connectionErrorMessage) { | |
| clearAll(); | |
| loadAll(); | |
| } | |
| } | |
| ); |
server/ui/src/message/Messages.tsx
Lines 30 to 34 in 43574a0
| React.useEffect(() => { | |
| if (!messagesStore.loaded(appId)) { | |
| messagesStore.loadMore(appId); | |
| } | |
| }, [appId]); |
This useEffect isn't reexecuted after the state is cleared by the reaction. On the current master version this is done in a different way that is reexecuted.

@material-uito@mui@uiw/codemirrorCloses #696
Fixes #695
Fixes #574
Fixes #762