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

Web: Modify components, define new types for yaml parse/stringify endpoints #40974

Merged
merged 8 commits into from
May 2, 2024

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Apr 27, 2024

part of https://github.com/gravitational/teleport.e/issues/3933

suggest reviewing by commit

This PR contains few specific changes contained in the commits:

  • We had a unused SlideTabs component that I tweaked to accept a isProcessing flag that when true, render spinners and prevent tabs from being clicked on. Refactored a bit to have the caller be responsible for keeping the index state.
  • Added async version of FieldSelectCreatable
  • Allow modifying fetched resources with useKeyBasedPagination: this is similar to the change made here. Resource listing can be stale when we apply CRU actions to certain resources because of caching. So instead of refetching list, we need to allow modifying the frontend cached list
  • Add boilerplate typing for some new endpoints defined in Add yaml parsing and yamilfying access monitoring rule in webapi #40936

This comment was marked as resolved.

@kimlisa kimlisa added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Apr 27, 2024
return (
<TabLabel
role="tab"
htmlFor={`${name}-${tabName}`}
onClick={() => setActiveIndex(tabIndex)}
onClick={e => {
e.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

Is this e.preventDefault() necessary? From what I see, this is just a <label>, so there's no extra behavior on click. Pointer events are turned off when it's processing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i was confused by it to, but without the e.preventDefault, onChange gets called twice for some reason

{tabContent}
<Box>
{selected && isProcessing && (
<Spinner delay="none" size="25px" />
Copy link
Member

Choose a reason for hiding this comment

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

Because of the absolute positioning, the spinner will get rendered outside of the given tab when the browser width is narrow enough.

Though I understand that this will be used only in a single place, so I guess it's good enough for now.

I suppose the issue was that you didn't want the text of the tab to jump when the spinner gets shown? I wonder if a horizontal progress bar would work here. Connect has one in LinearProgress.tsx, but we don't have stories for it unfortunately. I see that there's a story for "Animated Progress Bar", but this might be too big idk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the issue was that you didn't want the text of the tab to jump when the spinner gets shown?

yes

i'll add a note on this component to look into horizontal progress bar

@@ -69,7 +73,7 @@ type props = {
// The style to render the selector in.
appearance?: 'square' | 'round';
// The index that you'd like to select on the initial render.
initialSelected?: number;
activeIndex: number;
Copy link
Member

Choose a reason for hiding this comment

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

Good change. 👍 I mean controlling the current tab from outside of the component.

Could you update the comment and turn all comments into JSDocs?

Comment on lines +50 to +52
<FieldSelectCreatableAsync
inputId="test2"
label="FieldSelectCreatableAsync multi"
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't seem to ever show any options for me, even if I remove the filter in loadOptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch, i forgot to set defaulOptions={true} which loadOptions on initial render

@kimlisa kimlisa force-pushed the lisa/web-tweaks branch 2 times, most recently from f76985b to 2d193c7 Compare May 1, 2024 20:31
@kimlisa kimlisa force-pushed the lisa/web-tweaks branch from 2d193c7 to 7858881 Compare May 1, 2024 20:33
@kimlisa kimlisa enabled auto-merge May 1, 2024 20:33
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from rudream May 1, 2024 21:41
@kimlisa kimlisa added this pull request to the merge queue May 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 1, 2024
@kimlisa kimlisa added this pull request to the merge queue May 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 1, 2024
@kimlisa kimlisa enabled auto-merge May 2, 2024 04:52
@kimlisa kimlisa added this pull request to the merge queue May 2, 2024
Merged via the queue into master with commit 2af98c6 May 2, 2024
39 checks passed
@kimlisa kimlisa deleted the lisa/web-tweaks branch May 2, 2024 13:11
@public-teleport-github-review-bot

@kimlisa See the table below for backport results.

Branch Result
branch/v15 Failed

kimlisa added a commit that referenced this pull request May 2, 2024
…points (#40974)

* Web: update SlideTabs to render spinner depending on condition

* Web: Add async version of FieldSelectCreatable

* Web: Allow modifying fetched resources with useKeyBasedPagination

* Web: Add yaml parse/stringify api endpoints and types

* Web: Allow input search bgColor to take on parents bgColor

* Address CRs

* Update snaps and test
github-merge-queue bot pushed a commit that referenced this pull request May 3, 2024
…points (#40974) (#41148)

* Web: update SlideTabs to render spinner depending on condition

* Web: Add async version of FieldSelectCreatable

* Web: Allow modifying fetched resources with useKeyBasedPagination

* Web: Add yaml parse/stringify api endpoints and types

* Web: Allow input search bgColor to take on parents bgColor

* Address CRs

* Update snaps and test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants