-
Notifications
You must be signed in to change notification settings - Fork 144
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
Implement controlled list manager and reference datatype #10541 #10569
Conversation
arches/app/src/components/ControlledListManager/ControlledListInventory.vue
Outdated
Show resolved
Hide resolved
0d918ec
to
3e66b54
Compare
This comment was marked as outdated.
This comment was marked as outdated.
f27434d
to
4203f3c
Compare
arches/app/src/components/ControlledListManager/ControlledListTable.vue
Outdated
Show resolved
Hide resolved
arches/app/src/components/ControlledListManager/ControlledListTable.vue
Outdated
Show resolved
Hide resolved
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.
Overall looks great. There's a few things to hash out as patterns but a really strong start to laying the Vue groundwork 👍
Most things I flagged once but the request should be considered throughout ( eg no arches translations )
arches/app/media/js/views/components/plugins/controlled-list-manager.js
Outdated
Show resolved
Hide resolved
arches/app/src/components/ControlledListManager/SearchAddDelete.vue
Outdated
Show resolved
Hide resolved
arches/app/src/components/ControlledListManager/SidepanelDataView.vue
Outdated
Show resolved
Hide resolved
Thanks for the review--grateful to have your eyes on this! |
a5ce96c
to
9410ad1
Compare
a80dc7d
to
554e63e
Compare
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.
overall looks great, still a few things to work out && likely larger conversations to have. Of course this can be taken care of in smaller chunks in later mockup iterations as well.
const visible = ref(false); | ||
|
||
const { $gettext } = useGettext(); | ||
const slateBlue = "#2d3c4b"; // todo: import from theme somewhere |
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.
Now that #10616 has been integrated this opens up the conversation around theming.
By EOQ1 2024 the PrimeVue team will release v4, which has an updated theming architecture
Currently the roughed-out POC involves importing the stylesheets and manually editing the attrs based on selected theme.
Hopefully we can kick the can long enough for the updated version of PrimeVue so that we don't need to waste cycles on 2 solutions. However if need be we can drop theme-switching in the interim in favor of a css files that imports a primvue theme file and adds/overwrites values, which is then imported into base.htm
instead of the the theme css directly.
But while we're on the subject, regardless of architecture, what would populate the stylesheet? Colors for sure. Typography? Font-size? Border-radii? Seems we'd probably want to be on the 'light touch' end of the spectrum but I'm interested to hear your thoughts.
arches/app/views/controlled_lists.py
Outdated
from arches.app.utils.response import JSONErrorResponse, JSONResponse | ||
|
||
|
||
def serialize(obj, depth_map=None): |
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 probably opens up a larger conversation / can of worms but moving forward I'd like to move as much logic out of views as possible, and have them essentially just handle parsing args, calling other functions, and handling responses.
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.
I like it. So is the idea to break this up into three smaller functions and put them under /serializers?
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.
Broke up the serializer into model methods in fb740ae 👍
80a38ef
to
a205d5b
Compare
Thanks, those were really useful nudges re: PUT and re: the file hierarchy. I now have this, LMK what you think! (UPDATE: I since renamed the types for the arches object from arches.ts to types.ts) The one thing hanging out in plugins is the gluing code connecting knockout to vue, and ultimately just pointing to the ControlledListsMain. The
This is making me think the tests should live side by side with each file. |
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.
the API looks good!
3 small thoughts on the folder hierarchy:
-
I think it would save us potential headaches to have
arches/app/src/theme.ts
andarches/app/src/types.ts
moved toarches/app/src/arches/(types, theme).ts
, because of how webpack overwrites static assets that exist at the same path. -
can
src/controlledlists
be delineated? egcontrolled-lists
? -
I'm wondering how well this pattern will expand. Playing it out could result in
└── arches/
└── app/
└── src/
├── controlled-lists/
│ └── *stuff*
├── foo-report/
│ └── *stuff*
├── bar-widget/
│ └── *stuff*
├── baz-widget/
│ └── *stuff*
├── qux-card/
│ └── *stuff*
├── graph-designer/
│ └── *stuff*
├── arches/
│ ├── theme.ts
│ └── types.ts
├── plugins/
│ └── ControlledListManager.vue
├── reports/
│ └── fooReport.vue
├── widgets/
│ ├── barWidget.vue
│ └── bazWidget.vue
├── cards/
│ └── quxCard.vue
└── views/
└── GraphDesigner.vue
Which isn't great as the hierarchy is a breakdown of of application-level components and vue-components.
An alternative could be:
└── arches/
└── app/
└── src/
├── controlled-lists/
│ └── *stuff*
├── foo-report/
│ └── *stuff*
├── bar-widget/
│ └── *stuff*
├── baz-widget/
│ └── *stuff*
├── qux-card/
│ └── *stuff*
├── graph-designer/
│ └── *stuff*
└── arches/
├── theme.ts
├── types.ts
├── plugins/
│ └── ControlledListManager.vue
├── reports/
│ └── fooReport.vue
├── widgets/
│ ├── barWidget.vue
│ └── bazWidget.vue
├── cards/
│ └── quxCard.vue
└── views/
└── GraphDesigner.vue
where application-level delineation happens insde the arches/src/arches
folder. To me this is a bit better because it comes with de-facto namespacing, so that if someone wanted to write an arches application that overwrites fooReport.vue
, they'd need to consciously put it at my_cool_project/src/arches/reports/fooReport.vue
instead of my_cool_project/src/reports/fooReport.vue
. That could also save some headache of someone out in the wild accidentally overwriting a thing.
Though to me this is still not ideal, in the top level there's mixed breakdown: applications (eg arches) are mixed with componentry (foo-report, bar-widget).
Maybe a better answer is:
└── arches/
└── app/
└── src/
├── controlled-lists/
│ ├── plugins/
│ │ └── ControlledListManager.vue
│ └── modules/
│ └── *stuff*
└── arches/
├── theme.ts
├── types.ts
├── reports/
│ └── fooReport.vue
├── widgets/
│ ├── barWidget.vue
│ └── bazWidget.vue
├── cards/
│ └── quxCard.vue
├── views/
│ └── GraphDesigner.vue
└── modules/
├── foo-report/
│ └── *stuff*
├── bar-widget/
│ └── *stuff*
├── baz-widget/
│ └── *stuff*
├── qux-card/
│ └── *stuff*
└── graph-designer/
└── *stuff*
Where the top-level is delineated by application. The next level is broken down into application structure and a modules
folder where all the sub-components live. It's still not ideal because, say, what if I wanted to have an api.ts
file for both foo-report
and bar-widget
? I guess it could go in modules
but then where's the delineation between something like types.ts
?
But yeah I'm not sure if an ideal solution exists. But I want to hear your thoughts on this and see if we can't land somewhere close.
btw everything else looks good, once folder structure lands this is g2g in my opinion 👍
But then again I'm not sure. The CLM is technically it's own django app but it's still part of arches-core and the arches-core build process/bundle. maybe:
is best? With the idea that all project are now spawned with a |
Sure, I think that makes sense for the frontend. I was trying to keep 1:1 parity with
Maybe in #11072 we need to explore having a /src under arches/controlledlists. |
Your last suggestion is really appealing. Some items that occurred to me:
That gives this slight variation on your last suggestion (and all it requires me to do now is move plugins under arches, as you suggested):
What do you think? (We can still explore moving controlledlists up some more levels, outside of arches/app/src on #11072.) |
I like this layout of all the ones discussed. I like that the apps are segregated into their own directories at the level of arches and have their own hierarchies associated with them. |
Hmm this is closer but I'm still not sure. With
After giving it some thought I guess I'm leaning towards more closely emulating the folder structure outside of src. eg
Although @jacobtylerwalls with the work to move this out of arches-core, |
Yeah, my hope is to explore giving it its own /src folder in that PR, but as you know that requires fiddling a bit with webpack. Re: whether the graph designer is a view, maybe I picked a bad example, but I was trying to pick something that could be an app? What would a "view" be in this context? |
Yeah that should hopefully be a one-line change, and I'm happy to test that out && get it to you. One thing I hate is that it will de-standardize the webpack logic between projects and core. IMO nothing should be a Django app, not even this 😂 . But since we're here, I'd say nothing more should be broken out in this fashion. But for the sake of imagination, let's say search. If it gets broken out in the same fashion as the controlled list manager, it should still hopefully follow the same pattern of having its own |
I see where you're coming from. Another possibility is to not view "arches/app/src/arches" as "everything having to do with core arches" but more like "arches-router" where it's just a collection of single vue components (plugins, views, reports, whatever) that link out to other folders that sit beside arches. Then, if you take that mental model, you're less bothered by the idea of graph-designer sitting beside arches. That's where I was coming from. Maybe we'd want to rename arches -> arches-routes (or something better) in that case. In any case, I think we should keep innovating on this the next time we tackle a big vue feature, regardless of whether it's a distinct Django app or not. Sounds like we're talking about where to put future features, and finalizing this feature can happen on #11072. |
Yeah while I don't love the pattern, I hear you that it doesn't need to be discussed here in the main body of work. However I do feel we shouldn't punt this until the next large feature, as this is a good opportunity to solidify a pattern. So yeah I'm happy to approve here and hash out the directory structure in #11072 . |
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.
Types of changes
Description of Change
Implement the initial features for the Controlled List Manager.
TODOs:
Run migrations; then, to import some sample test data into your local db:
It's likely you'll need a
manage.py updateproject
also.Give your test user the
RDM_Administrator
group.Issues Solved
Closes #10541
Closes #10556
Checklist
Ticket Background
Demo
Component breakdown