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: plugin config param tooltips #30

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

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Nov 26, 2024

Resolves #24
Requires PRs in plugin repos be merged in and I'll remove the mocks

Changes:

  • tooltip for plugin param descriptions

QA:

image

@rndquu rndquu marked this pull request as draft November 27, 2024 15:50
Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

2 similar comments
Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

Copy link

@Keyrxng, this task has been idle for a while. Please provide an update.

@ubiquity-os-beta ubiquity-os-beta bot closed this Dec 15, 2024
@rndquu rndquu reopened this Dec 20, 2024
Copy link

@henalolp, this task has been idle for a while. Please provide an update.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 5, 2025

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 5, 2025

Beyond the tooltips I have implemented:

  • handling for various Typebox union schemas
  • displaying various toggles depending on the schema
  • Where unions are concerned, a tooltip with all descriptions etc, since those are rendered as a JSON object to edit vs input rows
  • I've tried to add as many helpful comments as I could to make it a bit easier for anyone else that works on this

I did not open a new task as I felt submitting this PR for review without the proper handling/displaying of descriptions etc was not meeting the spec and unions required additional handling, so I apologise for going off-spec a little.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 5, 2025

@gentlementlegen @0x4007 @rndquu - still unable to request a review so I'm having to tag, ty.

@Keyrxng Keyrxng marked this pull request as draft February 5, 2025 03:44
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 5, 2025

Since we are using markdown-it to render the descriptions we could mention it in plugin-template. I noticed text-convo-rewards with the all/exact/none items in the description use apostrophes vs backticks.

@Keyrxng Keyrxng mentioned this pull request Feb 6, 2025
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 9, 2025

@0x4007 @gentlementlegen I'm following up for review, ty.

@gentlementlegen
Copy link
Member

gentlementlegen commented Feb 9, 2025

I wanted to test the changes, but even if you didn't seem to have touched much logic the website for me is unusable. I have lots of errors on each screen and I cannot enter the plugin configuration anymore.

image

I cleared the cache, cleared the local storage, same result.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 9, 2025

@gentlementlegen I'm unable to reproduce as you can see the full flow below replicating what you did.


Majority of those errors I also get regarding fetching, maybe we can build an exclude list to avoid these being present in the console.

The errors that interest me are the configRepoExistenceCheck because it's telling you there that the .ubiquity-os repo cannot be found but then tries to create it and gets an already exists error (see here).

  1. Did you delete the .ubiquity-os repo and try to install from nothing?
  2. Did you delete just the file prior to testing? Or was it just empty?

Any other details you can give would help me to debug but I'm unable to repro

If you just renamed your config repo github does magic tricks and will reroute it, best to fully delete the repo.


Strange how from one review to the next it has managed to "break" without like you said any real changes to the logic. I'm trying to think what else could have changed on your side that's caused it since your last review things worked fine.

I can't actually zoom in on your screenshot either because the link is broken which doesn't help.

@gentlementlegen
Copy link
Member

gentlementlegen commented Feb 9, 2025

@Keyrxng I already have a configuration and my goal was to modify the plugins there. Made it public if that helps: https://github.com/Meniole/.ubiquity-os

A lot of errors come from trying to fetch https://github.com/mentlegen-2 which exists and is public, not sure why this happens.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 10, 2025

Thanks @gentlementlegen it helped a lot.

It's your config my friend, idk why exactly (needs another task to look into it) but if you try it with no repo or a config with far less going on inside of it then things work.

I'm able to repro and getting the same results with the same config.

image


  1. The errors seem to originate from the existence check despite the org and repo args being the same across config file contents I tested. Which doesn't correlate directly to whatever is inside the file that's breaking things.
  2. I've never seen this when using the installer to install/edit/remove so it's unlikely it'l arise from configs created from here.
  3. Like I said, I think this would need another task to really dig into what is going on because it's unrelated to the features of this PR.

@gentlementlegen
Copy link
Member

Created a related issue, will remove my current configuration and try again then.

@gentlementlegen
Copy link
Member

gentlementlegen commented Feb 11, 2025

@Keyrxng It works better after removing the configuration, although it takes a very long time to display the first screen because somehow it cannot fetch mentlegen-2 organization configuration. Logic also seems to work fine. Still a few remarks:

image

Why are some field names having dots and other having spaces?
Text box fields still seems not aligned properly
The commit name on config update starts with chore: Plugin which is not conventional commit friendly (should not start with uppercase)

Also it seems that these kind of selectors don't work:

Screen.Recording.2025-02-11.at.10.55.07.mov

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 11, 2025

Ty @gentlementlegen

because somehow it cannot fetch mentlegen-2 organization configuration.

This is a little hard for me to debug. Perhaps it's a permission/visibility issue with that org?

Why are some field names having dots and other having spaces?

Params which are objects use param.prop while standalone don't require it. In your example BaseRateMultiplier is standalone while the rest are child props. I felt this was the best way to display each vs a large JSON object which could be huge like text-conversation-rewards.Incentives.

Text box fields still seems not aligned properly

My mistake forgot to remove the margin should be fixed now:

Also it seems that these kind of selectors don't work:

Added a selected state

The commit name on config update

Reworded the commit to chore: updating config (Plugin Installer UI)

QA:

image

@gentlementlegen
Copy link
Member

Truly don't understand about that organization because it has very similar settings to the other ones, if I find something I'll let you know.
The problem to me is that during all the fetching the UI is completely empty, and the only feedback appears in the console.

Cool let me test again.

@gentlementlegen
Copy link
Member

One last margin problem remaining here:
image

Otherwise everything seems to work fine.

On the above screenshot, each of these have a lot of configurable parameters but only a text box is shown, when I thing there should be all the inner parameters available. This is obviously outside of the scope of this pull-request but it should be addressed within another pull-request.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 12, 2025

The problem to me is that during all the fetching the UI is completely empty, and the only feedback appears in the console.

Hmmm that's strange so for you, you don't see the toast notification saying that fetching is happening? Would a centered spinner or loader or something be prefered until all fetching has completed?

See I only have one org to fetch from these days so it's hard for me to experience having to fetch from 3 or 4 orgs which sounds like the case for you maybe.

image

QA:

image

On the above screenshot, each of these have a lot of configurable parameters but only a text box is shown, when I thing there should be all the inner parameters available. This is obviously outside of the scope of this pull-request but it should be addressed within another pull-request.

If I understand, you are suggesting that for these textareas which accompany the anyOf (union based selections) button groups, you'd prefer to render each child prop in the same way I do for a standard object using the param.prop convention?

Would we keep the buttons and collapse the rendered props for that union option if they disable it? And for those which cannot be disabled they'd stay rendered?

If you can create a clear spec for what exactly you'd prefer over this impl that would be great dude. I done it this way for a couple of reasons but I agree it's not optimal.

@gentlementlegen
Copy link
Member

Yes I can write a spec. My idea would be that all the fields should be rendered recursively within a tree, so if you have an object within an objects within an object they all render. I think this is doable since you have the full object shape within the generated manifest. But again outside of the scope of this pull-request.

also yes I have to render 5 organizations which is why it is slow. Ideally some spinner or anything that lets you know something is happening.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 13, 2025

One approval down, I'm unsure if we can merge this now or are we waiting for another? @gentlementlegen rfc please buddy

@gentlementlegen
Copy link
Member

Probably @0x4007 can take a peek and give some feedback

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 14, 2025

@0x4007 seems awfully busy but I hope this can move forward asap.

I'm free to work on other tasks I just need one of my previous to be merged.

Cheers @gentlementlegen

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Feb 14, 2025

@0x4007 if you don't have the time with upcoming events as you said, please assign another reviewer to get the 2nd approval needed for merge.

I'd trust @gentlementlegen's approval personally, the guy knows his shit but let's get this merged sooner rather than later, itching to move on.

@0x4007 0x4007 requested a review from whilefoo February 14, 2025 20:32
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 parameter descriptions for core plugins in the plugin-input.ts
3 participants