Complete rewite of the browser part (js, css) of the UI #165
Conversation
|
I forgot to mention that we tried to keep it iso-functional. This is the first step for possible future changes to the design and layout a bit to improve the usability. For testing purpose, in order to make sure that the information displayed in the interface is correct, it's possible to run 2 versions of powa-web at the same time. Running with the same repository data. In powa-web-dev-master:
image: powateam/powa-web-dev
container_name: powa-dev-web-master
command: "python3 powa-web-dev/run_powa.py"
ports:
- 8889:8888
volumes:
- '${POWA_WEB_MASTER_GIT}:/usr/local/src/powa-web-dev'
links:
- powa-repository
- remote-primary
- remote-standbyClone Then run containers with the following command: POWA_WEB_MASTER_GIT=../../powa-web-master POWA_WEB_GIT=../../powa-web POWA_COLLECTOR_GIT=../../powa-collector podman-compose -f powa-dev-demo.yml up |
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
2 times, most recently
from
6965a73 to
d6f479b
Compare
|
First, thanks again for working on that subjet. I don't really understand the PR though, I waited a bit to make sure that no more commits would be added and it looks like way more than simply the rewrite to get rid of some js stack and use a new one instead. I think that keeping the scope of this first change as small as possible would be easier to review. About the change itself, I tried to look at the new components but honetly I don't even understand what those are supposed to be used for based on their home pages. Naively, I see that you get rid of foundation (which I think is a good thing), but I was thinking that relying on bootstrap instead would be the logical replacement. As far as I can see it's not the case, and I'm a bit worried about that. Or is bootstrap part / dependency of some of the new components?
this is quite scary. There's no way to have the python server simply pass the js files like it was the case before? |
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
from
7563a2d to
ab982eb
Compare
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
2 times, most recently
from
cf67f20 to
624c48c
Compare
I wanted to propose this PR even though that there were some additional small fixes to be made. I could have waited a bit more but I wanted the team to have a look at the new look and feel. To be honest, I don't think it would have been possible to do get rid of only some js stack, ie. one at a time. The choice made :
Then as a consequence, the choice of ViteJS was straightforward. It replaces bower and is reponsible for the building and dependency management. Rickshaw which was used to display the graphs could have been kept at first even though compatibility hacks would have probably been required. I decided to get rid of it as well and replaced it with d3. In summary, it didn't seem possible to split this huge change into smaller steps. If this is a blocker I can of course consider splitting into smaller commits.
The only thing to know is that with VueJS it's now possible to have the component code all in one place (aka. Single File Components). In my opinion, it makes things a lot easier to understand.
In this rewrite, we don't use bootstrap at all. Instead we're using vuetify because it is more popular and has a more complete and up to date set of components available.
No, I don't think it's possible. |
|
Thanks for the commits that show how upgrading components is done! Could you also add some documentation (maybe a README.dev or similar) that points out the basics, like
And also document what needs to be done to actually host powa-web in a production style way (this one should eventually be documented in https://github.com/powa-team/powa/blob/master/docs/components/powa-web/deployment.rst but having it locally would still help). |
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
3 times, most recently
from
417d593 to
22878cb
Compare
I added a CONTRIBUTING.md file. Please let me know if it is clear enough and works for you. |
Thanks a lot, it looks great! After a quick look at it, I'm quite happy with the requirements and the general workflow, as nodes 16 doesn't look like a crazy recent version, and the various scenario needed for the common usages are straightforward (and way better than what they currently are). I will try to have a more thorough look at the PR shortly but it overall looks great! Again, I want to emphasize how grateful I'm for this work, congrats to all people involved. About merging that PR, and then all the other improvements you already made on the UI (which are also really welcome), I was thinking integrating that with the upcoming v5 version of powa that I'm already currently working on, which should hopefully be released somewhere around this summer. Would that work for you? As far as I can see there are only a few minor conflicts, which I can take care of if you prefer. If that's ok for you I can build and maintain specific "v5" versions of various podman images so you can easily work on that branch. The powa-archivist part is probably going to change quite often, but trashing it and recreating it regularly shouldn't be too much of a problem, especially since most of the changes that would impact the UI are already done. The reason for that is that this rewrite is a huge improvement. It will probably change quite a lot the general UI/UX so it would be weird to drop that in a minor version release. Also, I'm imagining that you may need a bit of time to adjust some details and/or add other changes, and since we only maintain a single major version it will give additional time for that while still having the possibility to release fixes for the current v4. |
Thanks.
Should I try rebasing the current branch onto your v5 branch in anticipation?
The earlier we can work on the UI rewrite on top of v5, the better. |
Assuming you're ok with the plan I mentioned sure!
I just pushed a new powateam/powa-archivist-git:v5 image. I think that's the only specific image you need to work on the v5, as you need a local copy of powa-web (on this branch rebased over v5) and powa-collector (on v5 branch). Let me know if it's working as intended.
Agreed. |
|
I tried running powa in dev mode with podman, but I got the following errors (found in container logs) in the 3 container running "powa-archivist:v5" image: FYI, I'm running the following command (from |
|
Ah, sorry I built the image on an arm machine and that's incompatible with an amd64 machine. I just pushed a new version, can you check if that solves the problem? |
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
from
22878cb to
4d1a423
Compare
|
Thanks. The current branch is now rebase on v5. It seems to work well. |
|
Great news! I will try to review the PR and test it thoroughly. We apparently hit different code path, and my local test setup heavily relies on the newest stuff changed in v5 so hopefully we should get a somewhat full manual testing of the UI between you and me. |
|
I think you used a wrong branch or something during the rebase. The CONTRIBUTING.md isn't there anymore, and the extra commits to bump the dependencies are also missing. |
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
from
4d1a423 to
c588ca0
Compare
|
Indeed. Fixed. |
|
Thanks. First initial feedback:
Maybe you forgot to git-add some file in the tree? I locally removed the pre-commit to be able to commit some fix, but I'm a bit worried about an undocumented pre-commit hook, especially since it can fail on something different that what it seems to be checking.
|
powa/static/js/components/Grid.vue
Outdated
| if (row.url) { | ||
| window.location.href = [row.url, serialize(store.from, store.to)].join("?"); | ||
| const destination = [row.url, serialize(store.from, store.to)].join("?"); | ||
| if (event.ctrlKey) { |
There was a problem hiding this comment.
Isn't it possible to just put an <a> there? A link is a basic html feature and I don't think there should be multiple lines of code to try to simulate something as basic as that. What about people who want to use alt click to open in a new window or use mouse middle click, or use something like vimperator and need a real <a> to do a batch action on those and so on?
There was a problem hiding this comment.
Also, doesn't this prevent users from only selecting some text to copy?
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
2 times, most recently
from
89d3ef3 to
691eba7
Compare
|
Thanks for the work on the changes marker. FTR one feature missing in the rewrite is when you click on the marker. In the older version, it makes the hover tooltip stay active and add a vertical line above (now should be below) the marker so you can easily eyeball the graphs before/after the change. Is that something that can be done with the rewrite? Note that one annoying thing in the current version is that the same marker for different graphs are independent. Ideally, we'd want to have to hover sticky thing and the vertical line be synced across all graphs. |
Everything now goes in .vue files. We don't need to manually declare the Vuetify components anymore. We now expose handler properties as JSON in template. We can't use VuetifyResolver for treeshaking because of the external component V-Snackbars. But we actually have our own resolver which does the same except that it excludes VSnackbars.
Display an error message only when main.js script fails loading.
The xhrm.html and messages.html templates and corresponding ui_method are not required anymore.
We always use <code> blocks to show code (SQL or explain plans). We don't use .hljs class since it adds extra styling we don't really want. Also we reset <code> font size (set by vuetify) since we don't want it to be smaller. In grid, we also reset some styling to make sure ellipsis work and background is not grey.
Otherwise it's loaded for every single component. We actually only want to override variables using sass preprocessor additionalData option.
We get rid to v-snackbars. Size is only handled with some flex positioning. Transition are preserved by making sure shown is set after creating the snackbar element.
dark_theme is stored in the localStorage to keep styling during the navigation. We have removed v-main classes in layout.html to manage the dark and light background on the v-application element. We copy the stackoverflow highlight dark and light css into a new highlight.scss file to dynamically manage highlight.css. We will be able to dynamically import the source file with SASS 1.64, but this will require Vuetify 3.
Not by using script tags in the HTML body. This will help loading new data without loading a new page.
By using vue-router (in HTML5 mode) we can reload the page content without reloading the page itself. The urls are still compatible with the old mechanism, which means that an url copied from the url toolbar will still work and display the same content. Disabled attribute for breadcrumb items is not required anymore since vuetify in combination with vue-router disables the item for the current route.
Update of vuetify help setting the text-transform with a scss variable. With this change only graphs legend and SQL <code> get a bit smaller.
We made it look more like the old one.
It's not supported at the moment.
This is required since we don't reload the page but use a single page. For example, the currently selected database may have changed and the action buttons may have changed too.
pgiraud
force-pushed
the
switch_backbone_vue_vite
branch
from
ed21a29 to
2349ebe
Compare
|
I just force pushed a fix for the popover. |
|
Great! I'm pretty happy with the state of the PR, I think we're ready to merge it when you want! |
|
And it's now merged! Thanks a lot for this awesome contribution @pgiraud, @MarionGiusti and @pirlgon (and my apologies if I missed anyone). |
|
@pgiraud some minor complain, I noticed that if multiple configuration changes are detected in the same snapshot, the timeline will show 2 events with the same timestamp (making it hard to spot and see) rather than merging them in the same hover tooltip. Is that something you can fix? |
With this PR the previous frameworks:
Are replaced by:
To test this, you have two options (yet to be documented):
npm run build. Built assets will be generated and put inpowa/static/dist[1]. Then run powa as you would normally do.npm run devin a terminal. This should launch a VITE service on http://localhost:5173/. Then run powa in dev mode in an other terminal.I'm looking forward for your feedback.
[1] the built assets are meant to be committed along with releases.