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

Improvements #25

Merged
merged 34 commits into from
Nov 28, 2024
Merged

Improvements #25

merged 34 commits into from
Nov 28, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Nov 21, 2024

Resolves #19

Changes:

  • readme display
  • production config only
  • reset to defaults

QA:

readme-with-improvements.mp4

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 21, 2024

This PR became quite long so I will address tests in another task

@rndquu @gentlementlegen if you'd like to take a look when you can, cheers.

@gentlementlegen
Copy link
Member

Seems to work fine although I have a few remarks:

  • I don't seem to have padding like in your video
image
  • the left panel shifts a bit when hovering the "use default" button
  • I am only seeing two orgs but I am in more orgs that that, how are they chosen?
  • going back from the edit plugin menu makes the bar take the full width
image - the "remove" button should maybe be disabled on plugins that are not installed

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 22, 2024

I am only seeing two orgs but I am in more orgs that that, how are they chosen?

https://docs.github.com/en/rest/orgs/orgs?apiVersion=2022-11-28#list-organizations-for-the-authenticated-user

public async getGitHubUserOrgs(): Promise<string[]> {
    const octokit = await this.getOctokit();
    const response = await octokit.rest.orgs.listForAuthenticatedUser();
    return response.data.map((org: { login: string }) => org.login);
  }

What orgs did you see?

I've considered other methods but the question is: What orgs do you want to see?

  • Orgs that you are an admin of or just owner of, collaborator of?
  • Should the partner determine which roles can edit the config or do we enforce rules, if we enforce then what should they be?

I think a task should be created and it discussed there because it'll involve additional work and direction.

We sort of scrapped global config variables from ubiquity-os-config.yml but we could leverage that to enforce partner defined roles which can edit (at least via the UI).

E.g: The org owner must use it after kernel-setup and we allow them to input roles that can edit and we write this to the config separate from plugins: and we read this on fetch and check against our authenticated user.

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 22, 2024

the left panel shifts a bit when hovering the "use default" button

I couldn't reproduce this pls lmk if it happens again and what you suggest as a fix/steps to repro, cheers

@Keyrxng Keyrxng mentioned this pull request Nov 22, 2024
@zugdev
Copy link

zugdev commented Nov 22, 2024

We should only show "no orgs available" after we've looked at the request answer, it can be confusing in high latency environments. Show "loading", either as string or animation. Perhaps copy from work.ubq.fi.

@zugdev
Copy link

zugdev commented Nov 22, 2024

  1. Though I've selected an org there is no visual indicator of what org I am at, this can be confusing.

  2. README fails to be opened for me. Also, no console.error log to help debug this.

image

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 22, 2024

README fails to be opened for me. Also, no console.error log to help debug this.

Any chance you wrote a log to find out as I cannot repro and I don't think it happened for mentlegen either or he'd have mentioned it?

I can add a console.error but would like to know the root cause as me adding a log won't resolve it, appreciate if you could.

We should only show "no orgs available"

I thought this notification was enough but I have updated the title for while it's fetching also

image

static/main.ts Outdated Show resolved Hide resolved
static/scripts/rendering/plugin-select.ts Outdated Show resolved Hide resolved
@zugdev
Copy link

zugdev commented Nov 22, 2024

These icons are not intuitive, perhaps text is better.

image

@rndquu rndquu marked this pull request as draft November 27, 2024 15:49
@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 27, 2024

@Keyrxng Check this screenshot:

The "Assistive pricing" plugin is disabled although its config is present in the .ubiquity-os.config.yml file. The expected behavior for the "Assistive pricing" is to be in the enabled mode.

Pls fix

My apologies @rndquu, fixed now

@Keyrxng Keyrxng marked this pull request as ready for review November 27, 2024 17:55
@Keyrxng Keyrxng mentioned this pull request Nov 27, 2024
@rndquu rndquu self-requested a review November 28, 2024 07:16
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

@Keyrxng

  1. For some of the plugins (User activity watcher, Automated merging, Telegram kernel) getting the No plugin URL found for user-activity-watcher error:
Screenshot 2024-11-28 at 10 24 57

As far as I understand those plugins work as github actions so it's not clear why we can't set them up.

  1. The command-ask plugin shows empty UI:
Screenshot 2024-11-28 at 10 29 42

Pls fix

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 28, 2024

Sorted @rndquu

  1. PRs opened in relevant repos: related to manifest.name mismatch #27
  2. I'm not able to reproduce this. Was it from a fresh cache fetch or did you have it installed?
    image

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 28, 2024

I merged ubiquity-os-marketplace/ubiquity-os-kernel-telegram#40 and kernel telegram is now working so the other PRs need merged and we should be good to merge this?

image

  1. Technically kernel-telegram is not built to support being installed by partners at the moment so should probs not be displayed.
  2. It has two config entries: A) I detect url(s), if plural, create an entry for each B) See links below

@rndquu rndquu self-requested a review November 28, 2024 16:33
@rndquu rndquu marked this pull request as ready for review November 28, 2024 16:33
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

Check this screenshot:
Screenshot 2024-11-28 at 19 41 55

Although the registerWalletWithVerification is set to true in the yml config the option in the html UI is not check while the expected behavior is to load current config values.

Pls fix.

@rndquu
Copy link
Member

rndquu commented Nov 28, 2024

Sorted @rndquu

  1. PRs opened in relevant repos: related to manifest.name mismatch #27
  2. I'm not able to reproduce this. Was it from a fresh cache fetch or did you have it installed?
    image
  1. So, as far as I understand, when Add parameter descriptions for core plugins in the plugin-input.ts #24 is solved all plugins setup will be fine because of the correct name property.
  2. I cleared the cache and error is gone

@rndquu rndquu marked this pull request as draft November 28, 2024 16:47
@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 28, 2024

Fixed, I had forgot to set checked relevant to the input value.

image

So, as far as I understand, when #24 is solved all plugins setup will be fine because of the correct name property.
I cleared the cache and error is gone

Yeah as can be seen with kernel-telegram after I merged. Excellent re: cache issue, I tried to improve the fetching of the readme because some would 404 maybe the manifest fetching needs made more robust if we see anything like that again

@rndquu rndquu marked this pull request as ready for review November 28, 2024 17:01
@rndquu rndquu self-requested a review November 28, 2024 17:01
@rndquu
Copy link
Member

rndquu commented Nov 28, 2024

Appears like you have solved the problems I found. The only thing I managed to find now is the overlap between table and header:

image

We could create a issue later for me to fix these kind of CSS problems. By taking a very quick look:

  1. Nav should be inside of header
  2. The height for content does not need to be calculated just use display block use the available height
  3. Fix overlapping
  4. Make responsive

I suppose when all of the critical issues are ready (#24, #22, #20) we can get back to neaten up the css.

@rndquu rndquu merged commit b52f2db into ubiquity-os:main Nov 28, 2024
4 checks passed
@rndquu
Copy link
Member

rndquu commented Nov 28, 2024

@Keyrxng Now you could merge the latest main branch into your other PRs from https://github.com/ubiquity-os/ubiquity-os-plugin-installer/pulls

@ubiquity-os-beta ubiquity-os-beta bot mentioned this pull request Nov 29, 2024
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.

UI improvements
4 participants