Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(ui): split arrays in yaml to fix ambiguous collapse when array items have nested objects #21064

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aali309
Copy link
Contributor

@aali309 aali309 commented Dec 4, 2024

Fixes: #20905

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Motivation

see link

Description of the change

Improve YAML array formatting in the UI to avoid ambiguous collapse behavior. When an array item contains nested objects, the dash (-) is now placed on its own line, making it clear which elements can be collapsed independently.

Copy link

bunnyshell bot commented Dec 4, 2024

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

@aali309
Copy link
Contributor Author

aali309 commented Dec 4, 2024

Before

1

in collapse

1b

After

2

in collapse

2b

@aali309 aali309 marked this pull request as ready for review December 4, 2024 22:13
@aali309 aali309 requested a review from a team as a code owner December 4, 2024 22:13
Copy link
Contributor

@keithchong keithchong left a comment

Choose a reason for hiding this comment

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

Note also that the user can edit the yaml and put the item on the same line as the dash.

Did you check if the formatting will affect the diff?

});

// Add newline after dash for nested objects
yaml = yaml.replace(/^(\s*)-\s+(\w+):/gm, '$1-\n$1 $2:');
Copy link
Contributor

@keithchong keithchong Dec 9, 2024

Choose a reason for hiding this comment

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

I think it'll be nicer if only the first item (which is a sub-array) is placed on the next line. As an example, from your screenshot, if you look at the element env starting at line 123, if you collapse them all, then each item will be 'blank' and it's not very meaningful. Something like this:

Screenshot 2024-12-09 at 1 04 09 PM

It'll be good if we can keep these items all on the same line, like before, so it'll be something like this:

Screenshot 2024-12-09 at 12 50 43 PM

What do you think about using these search and replace strings instead, respectively?

:(\s*)\n(\s*)-(\s+)(\w+):(\s*)\n(\s*)-

and

:$1\n$2-\n$2 $4:$5\n$6-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-12-09 at 4 52 23 PM

@aali309
Copy link
Contributor Author

aali309 commented Dec 10, 2024

@jsoref, please review this for me.

@jsoref
Copy link
Member

jsoref commented Dec 10, 2024

/bns:deploy

Copy link
Member

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

It seems good. I'm hoping to play with it, but that'd require someone to poke bunny

@aali309
Copy link
Contributor Author

aali309 commented Jan 13, 2025

@jsoref PING!

@crenshaw-dev
Copy link
Member

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

/bns:start

@jsoref
Copy link
Member

jsoref commented Jan 13, 2025

https://argocd-77zpnz.bunnyenv.com/applications/env-77zpnz/guestbook-77zpnz?view=tree&resource=&node=apps%2FDeployment%2Fenv-77zpnz-guestbook%2Fguestbook-ui%2F0

    spec:
      containers:
        - image: gcr.io/heptio-images/ks-guestbook-demo:0.2
          imagePullPolicy: IfNotPresent
          name: guestbook-ui
          ports:
            - containerPort: 80
              protocol: TCP
          resources: {}
          terminationMessagePath: /dev/termination-log
          terminationMessagePolicy: File
        - command:
            - /shared/argocd-dex
            - rundex
          env:
            - name: ARGOCD_DEX_SERVER_LOGFORMAT
              valueFrom:
                configMapKeyRef:
                  key: dexserver.log.format
                  name: argocd-cmd-params-cm
                  optional: true
          image: gcr.io/heptio-images/ks-guestbook-demo:0.2
          imagePullPolicy: IfNotPresent
          name: guestbook-ui-sidecar
          resources: {}
          terminationMessagePath: /dev/termination-log
          terminationMessagePolicy: File

I don't think it's doing what I want:
image

specifically, I can't collapse command without collapsing the entire sidecar container.

That said, I can't tell if this bunny deployment updated or not ☹️.

If someone could provide pictures/videos for the above, that'd be great.

@aali309 aali309 force-pushed the splitArrays branch 2 times, most recently from 2b5c729 to 1f6cb4c Compare January 29, 2025 20:11
@aali309 aali309 force-pushed the splitArrays branch 4 times, most recently from eefc34d to 0e9262b Compare February 14, 2025 17:17
@aali309
Copy link
Contributor Author

aali309 commented Feb 14, 2025

My.Movie.mp4

@jsoref, how about now?

…nline

Signed-off-by: Atif Ali <atali@redhat.com>
Signed-off-by: Atif Ali <atali@redhat.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split arrays in yaml so that the first line is blank to avoid messy collapse behavior
4 participants