-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: main
Are you sure you want to change the base?
Conversation
@Keyrxng, this task has been idle for a while. Please provide an update. |
@Keyrxng, this task has been idle for a while. Please provide an update. |
2 similar comments
@Keyrxng, this task has been idle for a while. Please provide an update. |
@Keyrxng, this task has been idle for a while. Please provide an update. |
@henalolp, this task has been idle for a while. Please provide an update. |
|
Beyond the tooltips I have implemented:
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. |
@gentlementlegen @0x4007 @rndquu - still unable to request a review so I'm having to tag, ty. |
Since we are using |
@0x4007 @gentlementlegen I'm following up for review, ty. |
@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
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. |
@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. |
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.
|
Created a related issue, will remove my current configuration and try again then. |
@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 ![]() Why are some field names having dots and other having spaces? Also it seems that these kind of selectors don't work: Screen.Recording.2025-02-11.at.10.55.07.mov |
This is a little hard for me to debug. Perhaps it's a permission/visibility issue with that org?
Params which are objects use
My mistake forgot to remove the
Added a selected state
Reworded the commit to QA: |
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. Cool let me test again. |
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. QA:
If I understand, you are suggesting that for these textareas which accompany the 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. |
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. |
Co-authored-by: Mentlegen <[email protected]>
One approval down, I'm unsure if we can merge this now or are we waiting for another? @gentlementlegen rfc please buddy |
Probably @0x4007 can take a peek and give some feedback |
@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 |
@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. |
Resolves #24
Requires PRs in plugin repos be merged in and I'll remove the mocks
Changes:
QA: