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.

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

feat: add show hidden option to outputs panel #1209

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

TomW1605
Copy link

@TomW1605 TomW1605 commented Oct 18, 2023

feat: add show hidden option to outputs panel

adds an option to the outputs panel that lets you show any outputs that have been hidden by putting _ at the start of their name.

i would like to add something that makes it so you can tell the "hidden" ones apart but that seems a bit more complex.

i ran this through the existing test and it passed everything but havnt had a chance to look at writing any new ones for this option

Signed-off-by: Thomas White [email protected]

@TomW1605 TomW1605 changed the title add show hidden option to outputs panel feat: add show hidden option to outputs panel Nov 26, 2024
@TomW1605
Copy link
Author

dont think im going to get around to adding the thing to display hidden ones differently but the _ at the start of the name should be enough. i also cant work out what tests are needed for this and cant actually find any existing tests for the ui to see what sort of stuff is being tested.

so im marking this as ready. if someone can point me at some existing ui tests or can give me guidance on what should be tested here i will work on that but i will do a new PR for the different look thing

@TomW1605 TomW1605 marked this pull request as ready for review November 26, 2024 07:22
@TomW1605
Copy link
Author

TomW1605 commented Dec 4, 2024

is there anything else that needs to be done to this before it can be merged?

@pedrolamas
Copy link
Member

Apologies, I just haven't had the time yet to review this, but will try to get that done before next release.

@TomW1605
Copy link
Author

TomW1605 commented Dec 4, 2024

no problem, just wanted to make sure there wasnt something i needed to do with it

Copy link
Member

@pedrolamas pedrolamas left a comment

Choose a reason for hiding this comment

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

I've finally managed to review this, in terms of code I have left a few things that need to change.

Functionality wise, this does make me a bit skeptical of its usage... Fluidd, Mainsail, and KlipperScreen have been moving towards the convention of hiding any and all elements that are prefixed with "_"

Users are aware of this, and the point is to allow to show/hide elements by using the configs, so in what scenario would this be used?

That also takes me to the other part which is that if we do this for Outputs, we probably should do the same for the rest, and that might not be as trivial as it was with Outputs here!

</v-list-item-action>
<v-list-item-content>
<v-list-item-title>
Show Hidden
Copy link
Member

Choose a reason for hiding this comment

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

Strings like this need to be localizable, so it should move to "en.yaml" and consumed with $t(app....) so it can be translated

Copy link
Author

Choose a reason for hiding this comment

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

yep, forgot to do that. will fix it this weekend

Copy link
Member

Choose a reason for hiding this comment

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

Lets also make this "Show hidden outputs" to make it more descriptive and distinct from any other.

Copy link
Author

Choose a reason for hiding this comment

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

agree

Copy link
Author

Choose a reason for hiding this comment

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

ok, got the other things done. where do you recomend putting this in the translation files?

src/components/widgets/outputs/OutputsCard.vue Outdated Show resolved Hide resolved
src/store/config/types.ts Outdated Show resolved Hide resolved
src/store/printer/getters.ts Outdated Show resolved Hide resolved
@TomW1605
Copy link
Author

I've finally managed to review this, in terms of code I have left a few things that need to change.

thanks for taking the time to look this over. they all seem resnable, i will make the changes and test them this weekend

Functionality wise, this does make me a bit skeptical of its usage... Fluidd, Mainsail, and KlipperScreen have been moving towards the convention of hiding any and all elements that are prefixed with "_"

Users are aware of this, and the point is to allow to show/hide elements by using the configs, so in what scenario would this be used?

i think there is a use case for this. mainly that it seems common to have outputs that you dont directly control 99% of the time that clutter up the interface so you hide them. this makes it easy to directly control these outputs without having to change the config files, something you can’t do at all while printing. it would also allow you to view non controllable outputs such as temp controlled fans that may be hidden much of the time as they are not normally something you would need to see.

That also takes me to the other part which is that if we do this for Outputs, we probably should do the same for the rest, and that might not be as trivial as it was with Outputs here!

i agree it would probably be useful for other things, though the main other things i can think of that use the _ hide system are temp sensors and macros. macros already have a way to hide and show them though fluidd without having to edit config files and that can be changed while printing and temp sensors can be hidden from the graph though adding this option to that widget as well would probably be useful. in the case of the temp widget there is already a "settings" dropdown with selectable options so UI wise adding this option would work fine. i have not looked at the code side to know how practical it is there but i dont think that should stop this being merged. happy to take a look at that too but i feel it should be a seprate PR.

@pedrolamas
Copy link
Member

I just gave this a quick test in my simulated environment, the outputs card is not showing, so something is broken!

@TomW1605
Copy link
Author

do you get any errors or anything? it seems to be working fine for me (both before and after the update merge i just did)

@pedrolamas
Copy link
Member

pedrolamas commented Dec 28, 2024

No errors, just the Outputs card missing... and it is indeed because of the get hasOutputs on Dashboard.vue: it is missing () on each of the store getters in use!

What I have no idea is how it is working for you!

@TomW1605
Copy link
Author

TomW1605 commented Dec 29, 2024

ok, got it fixed, and worked out why it was working for me.

i forgot that the remote dev stuff in webstorm dosnt auto push to remote when you do a git pull. so my testing was running a version without the check in Dashboard.vue

i have now ensured that the version on my server is fully up to date and tested it and everything is working

still not sure where to put the translation so havnt added that

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.

2 participants