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

Pre-campaigns API+UI changes #37

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

Pre-campaigns API+UI changes #37

wants to merge 9 commits into from

Conversation

HolecekM
Copy link
Collaborator

@HolecekM HolecekM commented Dec 13, 2024 •
edited
Loading

Description

(commit hashes now out of date because of rebase)

  • 9e1ad34: locally disable autopep8 to prevent moving imports in a way that would break eventlet's monkey patching again
  • 73cc5c0: allow configuration through env variables + README update (fixes [BUG] Readme how to run locally dashboard server does not work #34)
  • e377aad: show response body when showing an alert for server errors
  • d69e988: refactor sidebar to allow nested sections (CollapsibleSection)
  • a38fb9e: refactor and navigation state - instead of keeping chosenClass and chosenChallenge on App level, we now use window.location.hash-powered navigation, which:
    • preserves page on refresh
    • writes history entries when changing pages
    • allows better concern-separation in code, and thus will allow easier future expansion
  • 9eef781: persist open-close state of CollapsibleSection using localStorage
  • 2a0a178: previously, tasks were being returned from the database in an arbitrary order, meaning that they could show up in a different order than intended (specified in the challenge's meta.json) - this commit preserves the specification order by adding a new column and sorting by it

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • run_tests.sh
  • user smoke test

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • (If modifying dashboard client) I have checked my code for lints with
    eslint and formatted it properly using prettier
  • (If modifying dashboard server) I have checked my code for lints with
    pylint and formatted it properly using autopep8

@HappyStoic
Copy link
Collaborator

The classes were not working for me when I tried to click around the UI but after removing the duplicate <ClassDetail /> line, it started working.

Other than that LGTM!

I appreciate the use of stores, that's definitely a right way, I had no time to implement it so thank you!

HappyStoic
HappyStoic reviewed Dec 16, 2024
View reviewed changes
dashboard/client/src/SideNavBar.svelte Outdated
@@ -113,61 +44,46 @@
{#if visible}
<div transition:slide={{ axis: 'x' }} class="sidebar pt-5 flex-shrink-0">
<button type="button" class="btn-close close-btn" on:click={() => (visible = false)}></button>
{#if challenges === undefined || classes === undefined}
{#if !($challenges.length && $classes.length)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In here I think it's better to check for the undefined value. Happened already couple times that some challenges/classes broke the DB initialisation and so later the API then returned empty set of classes and challenges. In this case the UI would keep showing "loading" eventhough the loading was done, right? But of course that ideally should not happen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that is right - I'm changing the initial value to an explicit null and then checking for coercion to false rather than explicit equality with undefined, though

dashboard/client/src/Dashboard.svelte Outdated
<ClassDetail />
<ClassDetail curClass={$chosenClass} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a duplicate line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yes, the result of artificially splitting this into multiple commits >:)

@HolecekM HolecekM force-pushed the pre-campaigns-api-ui branch from 2a0a178 to 95174b0 Compare December 16, 2024 23:57
@HolecekM HolecekM requested a review from HappyStoic December 17, 2024 00:05
@HolecekM HolecekM mentioned this pull request Dec 17, 2024
Open
10 tasks
@HolecekM HolecekM changed the title Pre campaigns api UI Pre-campaigns API+UI changes Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HappyStoic HappyStoic Awaiting requested review from HappyStoic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Readme how to run locally dashboard server does not work
2 participants
@HolecekM @HappyStoic