-
Notifications
You must be signed in to change notification settings - Fork 16
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
Data Dictionary Version 2 #24
base: main
Are you sure you want to change the base?
Conversation
Might have to take this approach |
Was going through older DW issues and came across this: DW #1634 |
@foster33 I see, I am building this one from scratch to match some tickets that require new features like expandable rows. I am switching from Datatables to using Quasar + Vue. |
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 good overall, but there is still some additional work that needs to be done.
I was able to build and run your npm frontend manually, but for production we will want the frontend embedded into the microservice. So, you will have to update maven to install npm, build the project distributable bundle, embed that into the jar, and then configure spring to host that as the frontend instead of or in addition to the page we have traditionally hosted.
My recommendation would be to look at the get
method in the DataDictionaryController which is where the current HTML page is served.
I would also consider creating a new/additional DataDictionaryController with the base path set to 'data/v1'. That way, users can access either the older html page or the new one using the v2 api. This will help our users transition from the old interface to the new one.
For the frontend itself, I'm pretty happy with it, but I was able to find at least one edge case which produced strange behavior. This might be hard to explain via text, but i'll give it a shot...
With the sample data ingested by the quickstart there are three LANGUAGE
rows, each with different types and dates. One row is marked with the +
icon which causes it to expand to the full three rows. If you expand that group, and then search for a substring which is common to the unmarked rows, those rows will show up in the search. If you keep that group collapsed, and then search for the same substring (which doesn't match on the marked row) all three rows will be filtered out.
You can test this out against the LANGAUGE
field searching on 201
which matches the last updated field for the unmarked rows, but not the marked row. Hopefully that makes sense, but if not let me know.
Generally, if the filtering doesn't match on the marked row, but matches on the expanded rows, we should still show the expanded rows (and probably move the marker to first of the expanded rows).
EDIT:
We should make sure we are using this new style of frontend for the edge dictionary page as well, or make a ticket to do that at a later date. We can probably get away with punting on that if we keep the old pages under /v1
and the new page under /v2
Co-authored-by: jsduncan2 <[email protected]>
Changing variable types Co-authored-by: jsduncan2 <[email protected]>
Co-authored-by: jsduncan2 <[email protected]>
Adding comment to avoid confusion with Types Column
Co-authored-by: jsduncan2 <[email protected]>
Fixed Issues from the First run through of Data Dictionary and Squeezed out remaining bugs.
for (const row of rows) { | ||
let maxUp: number = row.lastUpdated; | ||
const fieldUp: any = row.internalFieldName; | ||
for (const scan of rows) { |
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.
If I'm correctly grasping what we're doing here, I think we can slightly optimize this by checking whether buttonValues
has fieldUp
. If it does we can skip this for-loop.
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.
Could you please clarify what you mean? buttonValues
is a Map with internalFieldName
and leastRecentlyUpdated
. The if statement on lines 67-73 updates the button element in the DOM if buttonValues
contains fieldUp
. Any insights on simplifying this would be helpful!
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.
What caught my eye was the double for-loop over rows
. I believe the overall objective for this block is to find the most recent row for the field. The inner for-loop seems to be running needlessly in the case we already found the most recent row for the current field.
Said another way, let's say fieldUp
is hamburger
on the first iteration of the outer for-loop and on the second it's also hamburger
. We've already found the most recent row for hamburger
so we can skip the inner for-loop.
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.
It's very possible I'm missing the intent of this code block. But usually looping over the same array twice within a single block indicates we could improve the algorithm.
46cf6ed
to
a7e8a25
Compare
Co-authored-by: jsduncan2 <[email protected]>
Creation of the Data Dictionary Version 2 Microservice:
npx quasar dev
in the /frontend library.Branch is called "Frontend" but also modifies the backend, with future updates planned for caching.