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.

Sign up for GitHub

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

Jump to bottom

scheme editor side panel #124

Merged
merged 4 commits into from
Dec 3, 2024
Merged

scheme editor side panel #124

merged 4 commits into from
Dec 3, 2024

Conversation

aarongundel
Copy link
Collaborator

No description provided.

@aarongundel aarongundel linked an issue Nov 12, 2024 that may be closed by this pull request
Add sidepanel to scheme page #100
Closed
@aarongundel aarongundel requested a review from chrabyrd November 12, 2024 18:45
@aarongundel aarongundel changed the title Adg/100 scheme editor side panel scheme editor side panel Nov 12, 2024
chrabyrd
chrabyrd requested changes Nov 15, 2024
View reviewed changes
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.

looks good! just a few nits

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Show resolved Hide resolved
arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Show resolved Hide resolved
arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Outdated Show resolved Hide resolved
@aarongundel aarongundel requested a review from chrabyrd November 18, 2024 19:35
@aarongundel aarongundel force-pushed the adg/scheme-report branch 2 times, most recently from 2a278d9 to 874b141 Compare November 21, 2024 16:44
@aarongundel aarongundel force-pushed the adg/100-scheme-editor-side-panel branch 2 times, most recently from 7ea336a to b141d12 Compare November 21, 2024 17:52
chrabyrd
chrabyrd requested changes Nov 21, 2024
View reviewed changes
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.

looks good overall, just a few minor things

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Outdated
<script lang="ts">
const MAXIMIZE = "maximize";
const SIDE = "side";
const CLOSE = "close";
</script>

<script setup lang="ts">
Copy link
Contributor

Choose a reason for hiding this comment

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

these two script tags should be combined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't; because these are used in defineEmits, I get an eslint check hook error when trying to commit that. There's a good explanation for that behavior here. Alternatively, I can do an eslint ignore as is mentioned in that post.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh fair. In that case I'd say either export them to constants.ts or don't use the variables in defineEmits. IMO we should avoid double <script> tags when possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reverted to strings, these feel more natural locally defined than in constants.

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Outdated
@@ -0,0 +1,64 @@
<script lang="ts">
const MAXIMIZE = "maximize";
Copy link
Contributor

Choose a reason for hiding this comment

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

these could arguably be moved to constants.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems odd, since the constant is only really used locally. If we were reusing it across multiple components, that would make more sense.

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Outdated
<div>
<h2>{{ $gettext("Scheme Editor Tools") }}</h2>
<div>
<Button @click="toggleSize"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably the formatter's fault, but IMO this should be either

<Button @click="toggleSize">

or

<Button 
    @click="toggleSize"
>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why the formatter originally allowed this to happen - but it did! Maybe there were more attributes on this button originally.

arches_lingo/src/arches_lingo/components/scheme/report/SchemeSection.vue Show resolved Hide resolved
@aarongundel aarongundel requested a review from chrabyrd December 2, 2024 22:43
@aarongundel aarongundel force-pushed the adg/scheme-report branch 2 times, most recently from 9605a1a to f9695ac Compare December 3, 2024 17:18
@aarongundel aarongundel force-pushed the adg/100-scheme-editor-side-panel branch from b138af1 to 2b96c94 Compare December 3, 2024 17:38
chrabyrd
chrabyrd requested changes Dec 3, 2024
View reviewed changes
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.

looks good! just one comment and should be g2g 👍

arches_lingo/src/arches_lingo/components/scheme/editor/SchemeEditor.vue Outdated
</template>
<style scoped>
.header {
background-color: #ddd;
Copy link
Contributor

Choose a reason for hiding this comment

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

background color should be some primevue variable so it can respond to theme changes

@aarongundel aarongundel requested a review from chrabyrd December 3, 2024 22:46
chrabyrd
chrabyrd approved these changes Dec 3, 2024
View reviewed changes
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.

lgtm! 🚀

Base automatically changed from adg/scheme-report to main December 3, 2024 22:56
@aarongundel aarongundel force-pushed the adg/100-scheme-editor-side-panel branch from 00ddb33 to d077e81 Compare December 3, 2024 23:00
@aarongundel aarongundel merged commit c198c7a into main Dec 3, 2024
5 of 6 checks passed
@aarongundel aarongundel deleted the adg/100-scheme-editor-side-panel branch December 3, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobtylerwalls jacobtylerwalls jacobtylerwalls left review comments

@chrabyrd chrabyrd chrabyrd approved these changes

Assignees

@chrabyrd chrabyrd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sidepanel to scheme page
3 participants
@aarongundel @chrabyrd @jacobtylerwalls