Skip to content

Conversation

@MonikaFu
Copy link
Contributor

@MonikaFu MonikaFu commented Dec 8, 2024

Depends on #108

Closes #131

In this PR:

  • icons are added to intro accordion titles
  • on open accordion is closed so that user sees an overview of all topics and the view fits into one page
  • user can close and open the tabs in accordion at will

@MonikaFu MonikaFu requested a review from jdhoffa December 8, 2024 14:13
@MonikaFu MonikaFu marked this pull request as draft December 8, 2024 14:13
@github-actions
Copy link

github-actions bot commented Dec 8, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://RMI-PACTA.github.io/pacta-dashboard-svelte/pr-preview/pr-139/
on branch gh-pages at 2024-12-09 20:36 UTC

@MonikaFu
Copy link
Contributor Author

MonikaFu commented Dec 8, 2024

I converted it to draft to prevent merging before #108 is merged.

@github-actions
Copy link

github-actions bot commented Dec 8, 2024

Docker build status

commit_time git_sha image
2024-12-09T20:35:46Z 124ed49 ghcr.io/rmi-pacta/pacta-dashboard-svelte:pr-139

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

@MonikaFu this PR implements two different things, can you please separate them into two PRs?

  1. Adding of icons (this is an easy approve and makes sense)
  2. The change of the accordion functionality warrants further discussion

More on 2. :

The auto_collapse option requires that one of the AccordionItem's is open by default (which you probably noticed).
I don't think removing the auto_collapse option is very user friendly... the Introduction is massive, and allowing the possibility of having all sections open at once (or, requiring that the user shuts each section and opens the next one after they are done) seems against the goal of "having it all fit in one view" of the dashboard.

I understand (and do tend to agree) that having all sections collapsed by default is a bit sleeker, but I don't think it's worth doing that at the expense of losing autocollapse

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

For example, see how long this scroll bar becomes when all sections are opened at the same time:
Screenshot 2024-12-09 at 13 56 54

The autocollapse option would make it impossible for the page to ever get so long

@jdhoffa
Copy link
Member

jdhoffa commented Dec 9, 2024

With #108 merged, I see that the icons now render as expected here 😊
(looking at the PR preview link)

@jdhoffa
Copy link
Member

jdhoffa commented Dec 10, 2024

Note #150 handled the incorporation of icons into the accordion.
Recommend closing this PR, and opening a separate PR to remove the autocollapse functionality (if that's desired)

@MonikaFu MonikaFu closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🎨 add icons to accordion menu

3 participants