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

Integrate hierarchy view into schemes list placeholder #18, add concept and scheme routing #15 #86

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Aug 29, 2024

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:

  • Language switcher not yet implemented
  • Some components (e.g. <PresentationControls>) are just imported from arches-references instead of reimplemented. Others e.g. <TreeRow> are reimplemented. I chose not to namespace them under arches_references under arches_lingo because I had no higher higher level components from arches_references that needed to resolve to them.
  • Styling not finalized
  • Once we know what other pages want this hierarchy, we can lift some of the state up (e.g displayed row).

Base automatically changed from jtw/search-backend to main September 5, 2024 18:23
@jacobtylerwalls jacobtylerwalls changed the base branch from main to 19-cbyrd-implement-basic-search September 5, 2024 20:19
@jacobtylerwalls jacobtylerwalls changed the title Integrate hierarchy view into schemes list placeholder #18 Integrate hierarchy view into schemes list placeholder #18, add concept routing Sep 6, 2024
@jacobtylerwalls jacobtylerwalls changed the title Integrate hierarchy view into schemes list placeholder #18, add concept routing Integrate hierarchy view into schemes list placeholder #18, add concept and scheme routing #15 Sep 12, 2024
@jacobtylerwalls jacobtylerwalls changed the base branch from 19-cbyrd-implement-basic-search to main October 23, 2024 18:21
@jacobtylerwalls jacobtylerwalls changed the base branch from main to 19-cbyrd-implement-basic-search October 23, 2024 18:22
@chrabyrd chrabyrd force-pushed the 19-cbyrd-implement-basic-search branch from c1a4804 to 093d808 Compare October 28, 2024 18:49
Base automatically changed from 19-cbyrd-implement-basic-search to main October 30, 2024 13:26
vitest.config.mts Outdated Show resolved Hide resolved
Copy link
Contributor

@chrabyrd chrabyrd left a 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/App.vue Outdated Show resolved Hide resolved
arches_lingo/src/arches_lingo/api.ts Show resolved Hide resolved
Comment on lines 9 to +8
export const ANONYMOUS = "anonymous";
export const ERROR = "error";
export const SECONDARY = "secondary";
export const CONTRAST = "contrast";
Copy link
Contributor

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?

Copy link
Member Author

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.

arches_lingo/src/arches_lingo/utils.ts Show resolved Hide resolved
arches_lingo/templates/arches_urls.htm Show resolved Hide resolved
Copy link
Contributor

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:

Screenshot 2024-10-31 at 2 40 47 PM

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 👍

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Comment on lines 54 to 69
<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>
Copy link
Member Author

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?

@jacobtylerwalls
Copy link
Member Author

Thanks for the review! I think the future of a couple things need to be fleshed out a little further before working on them.

</script>

<template>
<div class="subheading">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to factor this out into a component once we know whether it should also be the component that contains the Toggle Hierarchy button. (I don't think we know that yet?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants