-
Notifications
You must be signed in to change notification settings - Fork 0
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
Integrate hierarchy view into schemes list placeholder #18, add concept and scheme routing #15 #86
Conversation
28dac8e
to
89ef29f
Compare
cf93a53
to
8ca344b
Compare
1757ca7
to
5a316c0
Compare
c1a4804
to
093d808
Compare
023e2ba
to
d4aff32
Compare
1507ef2
to
f44b22e
Compare
5675ebb
to
0557f7a
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 good! Just a few things. Also one thing I noticed is that pulling in main
results in some merge conflicts 😢
arches_lingo/src/arches_lingo/components/basic-search/SearchResult.vue
Outdated
Show resolved
Hide resolved
export const ANONYMOUS = "anonymous"; | ||
export const ERROR = "error"; | ||
export const SECONDARY = "secondary"; | ||
export const CONTRAST = "contrast"; |
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 that it needs to be handled in this review, but the more I think on it the more I think basic string mapping should stay at the component level. Slims downs imports && it's the same number of lines. WDYT?
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.
That's fine, and honestly for strings that are primevue constants, I'm not sure we need to defend against typos so aggressively, I'd be fine with just inlining them. It's the custom strings I like seeing pulled out.
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.
A few things here:
From my latest understanding of the mockup, the tree is a drawer not a splitter:
Now this may change, or we may decide a splitter is better, so not married to the idea but just pointing it out.
One thing the mock is showing is that the tree component is accessed by clicking a button in the subheader on many different pages. To me this says it's a lazy loaded component that should be entirely ( or near entirely ) contained, so it doesn't leak logic like setDisplayedRow
to the components where it's called. Definitely has room for interesting prop and/or emit patterns to signal updates. This also sorta goes back on the idea that was discussed during sprint planning ( or standup? I forget ) about only showing one scheme in the tree at a time. One would imagine that if you're on the Schemes list page you'd want to be able to open the tree and see a list of all schemes.
Lastly this may just be a naming issue, but I don't love the idea of Base
components as frontend patterns. Very similar to templates and IMO contrary to component decomposition. I visualize the concept and scheme pages similar to:
// pages/Concept.vue AND pages/Scheme.vue
<div>
<SubHeader />
<div>
... stuff specific to the page
</div>
</div>
with the Subheader containing the Tree and maybe a listener that tells the tree when to re-render. This is definitely an oversimplified example. Happy to chat to flesh out the idea more if need be 👍
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 sounds like it needs some discussion and a follow-up ticket. The splitter was just to get something on the page for the demo.
Lastly this may just be a naming issue, but I don't love the idea of Base components as frontend patterns. Very similar to templates and IMO contrary to component decomposition. I visualize the concept and scheme pages similar to:
I'm okay with that, but I do think we should revisit this once we know what we're doing re: drawer/list of schemes :P
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.
a follow-up ticket
Or better, just repurpose #111.
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.
Removed "base" from the file name. I see the subheading becoming its own component, but I guess I'd rather not polish that until we know where the toggle button goes, otherwise that's a lot of state to pass around before firmed up.
<div class="subheading"> | ||
<h2 v-if="displayedRow"> | ||
{{ | ||
getItemLabel( | ||
displayedRow, | ||
selectedLanguage.code, | ||
systemLanguage.code, | ||
).value | ||
}} | ||
</h2> | ||
<Button | ||
:severity="shouldUseContrast() ? CONTRAST : SECONDARY" | ||
:label="$gettext('Toggle hierarchy')" | ||
@click="() => (showHierarchy = !showHierarchy)" | ||
/> | ||
</div> |
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'm sure this will become its own component, but I don't want to polish it until we know what we're doing. Will there be a toggle button? Does this live in a drawer?
Thanks for the review! I think the future of a couple things need to be fleshed out a little further before working on them. |
6253eb9
to
7e0d0c8
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.
LGTM! 🎸
Thanks for the review! |
Visit http://127.0.0.1:8000/schemes to see the hierarchy component on the left in a collapsible panel.
Most of the tree functionality is borrowed from the controlled list manager.
Closes #15
Closes #18
Closes #114
For discussion:
<PresentationControls>
) are just imported from arches-references instead of reimplemented. Others e.g.<TreeRow>
are reimplemented. I chose not to namespace them underarches_references
underarches_lingo
because I had no higher higher level components from arches_references that needed to resolve to them.