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

fix/ui quirks #1259

Merged
merged 4 commits into from
Aug 5, 2024
Merged

fix/ui quirks #1259

merged 4 commits into from
Aug 5, 2024

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Aug 2, 2024 •
edited
Loading

A few fixes collected locally:

  • fix: Avoid flickering loading state
    • We always issue 3 requests to load tables/shared views/contexts, now before loading state was weirdly shared, now there is one state per request and we show a combined loading indicator. In the future we could think about just having one endpoint for this in the backend
  • fix: No console errors for elements that could not be found (fix TypeError: Cannot read properties of null (reading 'classList') #1255)
  • fix: Only render sidebar if visible
    • Otherwise we always do an extra request to fetch shares (even if we don't show or need it)
  • fix: Avoid full table/view list reloading on insert
    • If we just create a new table there is no need to do a full reload of the lists from the server

@juliushaertl juliushaertl added bug Something isn't working 3. to review Waiting for reviews labels Aug 2, 2024
This was referenced Aug 2, 2024
Open
Closed
enjeck
enjeck approved these changes Aug 5, 2024
View reviewed changes
Copy link
Contributor

@enjeck enjeck left a comment

Choose a reason for hiding this comment

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

Looks great. Just one question

src/App.vue Outdated
Comment on lines 102 to 104
const currentlyActive = document.querySelector('header .header-left .app-menu li.app-menu-entry--active')
currentlyActive?.classList?.remove('app-menu-entry--active')
targetElement?.classList?.add('app-menu-entry--active')
Copy link
Contributor

@enjeck enjeck Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With these optional chains, we'll never notice any errors if the class name changes again and the active navigation styling stops showing? Do we want that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, good catch, let me skip those

@juliushaertl
fix: Avoid flickering loading state
cebc6eb 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
fix: No console errors for elements that could not be found
bbd3459 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
fix: Only render sidebar if visible
d59e9c2 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
fix: Avoid full table/view list reloading on insert
d2b2086 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl merged commit 0123c60 into main Aug 5, 2024
45 of 46 checks passed
@juliushaertl juliushaertl deleted the fix/ui-quirks branch August 5, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enjeck enjeck enjeck approved these changes

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

@elzody elzody Awaiting requested review from elzody

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read properties of null (reading 'classList')
2 participants
@juliushaertl @enjeck