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

GH-1306, GH-1443, JENA-2307: Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes #1307

Merged
merged 8 commits into from
Aug 14, 2022

Conversation

kinow
Copy link
Member

@kinow kinow commented May 15, 2022

part of #1306

closes #1443

  • b-alert
  • b-badge
  • b-btn
  • b-button
  • b-card
  • b-card-body
  • b-card-header
  • b-card-title
  • b-col
  • b-collapse
  • b-container
  • b-form
  • b-form-group
  • b-form-input
  • b-form-radio-group
  • b-form-select
  • b-input-group
  • b-input-group-append
  • b-list-group
  • b-list-group-item
  • b-nav
  • b-navbar
  • b-navbar-brand
  • b-navbar-nav
  • b-navbar-toggle
  • b-nav-item
  • b-overlay
  • b-pagination
  • b-popover
  • b-progress
  • b-row
  • b-skeleton
  • b-spinner
  • b-table
  • b-textarea
  • b-th
  • b-tr

@kinow kinow self-assigned this May 15, 2022
@kinow kinow marked this pull request as draft May 15, 2022 10:11
@afs
Copy link
Member

afs commented May 15, 2022

Awesome - the simpler (less dependencies) the better.

@kinow
Copy link
Member Author

kinow commented May 22, 2022

- Me: @afs do not worry, this update is going very well
- Also me:
GIFrecord_2022-05-22_153237

😄

@kinow
Copy link
Member Author

kinow commented May 22, 2022

Fixed a bug in the Edit, where a recent bug fix in the UI made it raise an error whenever we loaded the page. The issue was the order the components were loaded, so I just had to hook the reactivity another way (watch a property instead of doing when the component is created).

@kinow
Copy link
Member Author

kinow commented May 22, 2022

Added the CSS of Bootstrap 5 directly after the Bootstrap Vue. It looks like Bootstrap Vue was either using an older Bootstrap 5, or customizing some classes. Now the layout has a few problems, but we can fix it as we update the components.

Once all components here have been migrated to Bootstrap, my plan is to remove the CSS styles and JS dependencies of Bootstrap Vue in this PR, and then adjust the layout to match what we have (it will be slightly different, font color may be a darker black, corners a little more or less rounded, and other cosmetic changes due to changes in Bootstrap, but nothing major, in theory).

@kinow
Copy link
Member Author

kinow commented May 29, 2022

form-group was dropped in 5.2.
image

@kinow
Copy link
Member Author

kinow commented Jul 9, 2022

Now pending just the table element. It's the most elaborate from the ones we used, so this one might take a bit longer.

@kinow
Copy link
Member Author

kinow commented Jul 15, 2022

Finished with b-table, missing now just the Pagination component, and some testing.

I fixed one Vue reactivity issue in the query page. Will have to confirm, but it may be #1443, or related to it.

@afs
Copy link
Member

afs commented Jul 15, 2022

Sounds like this is in a good position.

@kinow
Copy link
Member Author

kinow commented Jul 17, 2022

All of the BootstrapVue components have been converted. Now just missing

  • remove the b-table from the upload view, use the new table component
  • remove any left-over BootstrapVue code
  • remove BootstrapVue
  • find a replacement for the Toaster component of Bootstrap Vue (argh), used for notifications/alerts
  • manual tests
  • fix existing tests

@kinow kinow force-pushed the vanilla-bootstrap branch 2 times, most recently from f576b12 to a30cad6 Compare July 19, 2022 07:29
@kinow kinow marked this pull request as ready for review July 19, 2022 11:48
@kinow

This comment was marked as outdated.

@kinow
Copy link
Member Author

kinow commented Jul 20, 2022

Managed to add e2e tests today too. They run similar to Selenium, but due to the way Cypress works, tests are less brittle.

Running the e2e tests will download some large dev dependencies, and video recording is disabled by default to reduce disk space, but that can be enabled to record the tests if needed.

yarn run coverage:e2e
yarn run test:e2e
yarn run test:e2e -- --headless --browser chrome

These are examples of how to run it. Without the --headless, Cypress will display a dialogue to choose the browser to run the tests with. You can go back in time and review what the UI looked like for each step of the test, which is quite handy when reproducing tests or user issues.

I've implemented a small JS mock server to store datasets and return some dummy data for the UI. This way we don't need to start a Docker container or something like that every time we run the tests (I found out that that helps speeding up the UI development, especially if a contributor is not familiar how to run the Jena backend.)

Screenshot from 2022-07-20 13-26-10

Test coverage is also quite good now with the e2e tests - the report below shows just the e2e tests coverage, but what's important is that it's covering the new code that I had to write to move away from Bootstrap Vue, and also that we can use these tests to write most of the new tests if needed (i.e. easy to copy-and-paste to create new tests).

Screenshot from 2022-07-20 13-57-08

Ready for review @afs, whew. After this one gets merged, my next task will be to upgrade all dependencies to the latest, and start moving things from Vue 2 to Vue 3. I will need to refresh my memory on what's new, and follow their migration guide. But it should be a lot easier than it was to remove the Bootstrap Vue dependency from our code base.

Thanks!
-Bruno

@kinow kinow requested a review from afs July 20, 2022 02:00
@kinow
Copy link
Member Author

kinow commented Jul 20, 2022

@ramsel1508, my last commit contains an e2e test for your issue #1443. Hopefully that will prevent this issue from happening again 👍 Thanks for reporting and for the patience!

-Bruno

@kinow kinow changed the title Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes GH-1306: Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes Jul 20, 2022
@kinow kinow changed the title GH-1306: Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes GH-1306, GH-1443: Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes Jul 20, 2022
@kinow kinow mentioned this pull request Jul 21, 2022
@kinow kinow changed the title GH-1306, GH-1443: Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes GH-1306, GH-1443, JENA-2307: Replace Bootstrap Vue by vanilla Bootstrap 5 CSS classes Jul 21, 2022
@kinow
Copy link
Member Author

kinow commented Jul 21, 2022

Rebased. Then re-ran yarn install. Unit tests, and e2e tests both have passed successfully :-)

Closed the superseded issue for e2e tests, and linked this one in JIRA as well.

@kinow
Copy link
Member Author

kinow commented Jul 22, 2022

Started migrating the app to Vue3 to save time (not committing anything here, will stash / create a diff).

Only blocker so far: vuejs/router#454

Other than that, got the app up and running with no warnings after poking around with yarn, my IDE, and reading the great migration docs the Vue devs wrote 🎉

@afs
Copy link
Member

afs commented Jul 28, 2022

I downloaded the PR as a diff, applied it to current main (2022-07-28), and did a local build.

Only two things, one is quite minor:

The edit function: it is not loading the current contents of the graph when you click on the graph in the list of graph (neither default nor named). This means the edit are has no triples, and when put back the graph is empty.

When uploading data ("add data") there is a flash of orange in the progress bar then it goes green as it advances (guess: the default is orange). Minor.

Otherwise looking good and everything I tried worked.

@kinow
Copy link
Member Author

kinow commented Jul 28, 2022

Shouldn't be hard to address these two issues. Will update it over the next days. Thanks @afs !

@kinow
Copy link
Member Author

kinow commented Aug 2, 2022

When uploading data ("add data") there is a flash of orange in the progress bar then it goes green as it advances (guess: the default is orange). Minor.

I think that's because either your data is really small, or your server is responding really fast. Here's what it looks like when I upload a ~3MB file (colors are a little off due to recording quality).

GIFrecord_2022-08-02_203732

It has no color by default. After you have selected a file and clicked upload, it will start the upload and change the bar color to orange and increase the progress status/number. Once it reaches 100% and the upload is complete it should turn green.

…yellow warnings to be displayed for a quick moment before the page is fully loaded)
@afs
Copy link
Member

afs commented Aug 2, 2022

Yes - it was on the local machine and a very small test file. I assumed orange was an "error indicator", not "in-progress".

@kinow
Copy link
Member Author

kinow commented Aug 2, 2022

Yes - it was on the local machine and a very small test file. I assumed orange was an "error indicator", not "in-progress".

Good point! Follow-up to improve it: #1467

@kinow
Copy link
Member Author

kinow commented Aug 2, 2022

The edit function: it is not loading the current contents of the graph when you click on the graph in the list of graph (neither default nor named). This means the edit are has no triples, and when put back the graph is empty.

Excellent feedback.

There was another issue that when you clicked on add data or edit, you could see a yellow-ish box for a very quick moment, which says that No Service was found for the graphs. That's because the page is still loading the services from the service (i.e. an HTTP request and checking if we have the gsp-rw, and other services).

I changed it and now it should display that message only after it has finished loading the graphs and indeed couldn't locate the services. Before that we should see only a normal Loading/Spinner, or a Placeholder element until the page is fully loaded.

Now for the issue you found, there were two problems. One being a JavaScript error (thanks for catching that up!), and another left-over from the Bootstrap Vue migration 😮 I've removed it, fixed the JS error, and wrote an e2e test to try to prevent it from happening again 👍

@afs in case you haven't seen the e2e tests yet, here's what it looks like when I re-run the e2e test for the Edit function. The HTTP calls are mocks that I copied/adapted from a running Fuseki instance. It creates 11 datasets (for testing pagination), and selects the first a dataset, then lists the current graphs, and clicks on the graph name. Finally, it confirms the code editor contains some expected text.

GIFrecord_2022-08-02_221041

After the test has completed, you can "go back" in time. It keeps "frames" of the UI (hidden elements in the DOM). As you scroll back in the test timeline, it shows the past of the execution, allowing you to see what was clicked, what was displayed, etc.

I'm hoping this will help us move with the UI without adding regressions in every release (No promises! JS is a really permissive language, and there are so many libs involved, even though we are trying to be minimalistic... but that's the theory 😆 )

Ready for review again! Thanks! 👋

@afs
Copy link
Member

afs commented Aug 13, 2022

Just checking here - subject to review, this is now ready to go into the codebase? If so - great - 4.6.0 here we come!

And now PR review ...

@kinow
Copy link
Member Author

kinow commented Aug 13, 2022

Yup, just pending a review with some testing.

If you find any other pending things here it shouldn't be hard to fix, and the best part is that this PR has added e2e tests.

So in case of issues/regressions, we can cover pretty much any issue with unit or e2e tests.

I have another branch with vue 3 on top of this PR. Just pending one small issue. So for next release I think we should be able to update all our dependencies to their latest too.

Thanks!

@afs afs merged commit 27bb115 into apache:main Aug 14, 2022
@afs afs mentioned this pull request Aug 14, 2022
@kinow kinow deleted the vanilla-bootstrap branch August 14, 2022 21:08
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.

Fuseki Query-UI sends request to wrong endpoint
2 participants