-
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
Scheme report rough out #115
Conversation
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.
👋 @aarongundel !
Are you able to clean up this PR? There seems to be some leakage from Jacob's work. I'm fine if this is based off of that branch
rebased to merge into Jacob's branch. |
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.
Good rough out! Just a few notes 👍
titleText: string; | ||
}>(); | ||
|
||
const buttonText = `Add ${props.titleText}`; |
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.
needs i18n, also if it's just display text it should be inlined
|
||
<style scoped> | ||
.section { | ||
margin: 0 20px; |
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.
for Vue development we're moving away from sizing via px
, and instead using rem
to better support accessibility and smaller screens 👍
import SchemeUri from "@/arches_lingo/components/scheme/report/SchemeUri.vue"; | ||
import SchemeStandard from "@/arches_lingo/components/scheme/report/SchemeStandard.vue"; | ||
import SchemeAuthority from "@/arches_lingo/components/scheme/report/SchemeAuthority.vue"; | ||
import Splitter from "primevue/splitter"; |
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.
nit: vue/primevue imports above arches imports
@@ -0,0 +1,7 @@ | |||
<script setup lang="ts"> | |||
import SchemeReportSection from "./SchemeSection.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.
all imports should use the @/path
pattern
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 the idea of this component, but it may need to go away in further iterations as cardinality logic with these section components may prove tricky
Great feedback, thanks! |
09c1a24
to
b82dfa0
Compare
2a278d9
to
874b141
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.
looks good! Just a few more things have fallen out 👍
titleText: string; | ||
}>(); | ||
|
||
const buttonText = $gettext(`Add ${props.titleText}`); |
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.
gettext interpolation needs to be updated
https://jshmrtn.github.io/vue3-gettext/functions.html#interpolation
Also IMO you should inline this instead of declaring it.
.section .header { | ||
display: flex; | ||
align-items: center; | ||
border-bottom: 1px solid #ddd; |
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.
IMO this should change to reusable var: var(--p-menubar-border-color);
<div> | ||
<Splitter> | ||
<SplitterPanel> | ||
<SchemeNote /> |
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.
nit: these don't need to be hardcoded, they can be used in a v-for loop like SideNav
items
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 will punt on this until the adg/125-namespace-scheme
PR where this is done.
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.
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 more I look at this, the more I think this is a change for App.vue rather than the scheme report itself.
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.
cool, fair 👍
e65f279
to
403ab8d
Compare
arches_lingo/src/arches_lingo/components/scheme/report/SchemeSection.vue
Outdated
Show resolved
Hide resolved
9605a1a
to
f9695ac
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.
looks good! One minor thing with the splitter and it should be g2g 🎉
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.
cool, fair 👍
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! 🚀
6253eb9
to
7e0d0c8
Compare
d5280ed
to
dc9c36f
Compare
Initial attempt on roughing out scheme report