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

Move the new role editor to a full-screen dialog #49881

Merged
merged 16 commits into from
Dec 10, 2024

Conversation

bl-nero
Copy link
Contributor

@bl-nero bl-nero commented Dec 6, 2024

To turn on the new UI, go to developer tools -> Application -> Local storage and add a grv_teleport_use_new_role_editor key set to true. This will be lifted once UI is good to be released.

Demo
Figma

Requires #49801
Contributes to #46612

@github-actions github-actions bot requested a review from avatus December 6, 2024 15:21
@github-actions github-actions bot requested a review from kiosion December 6, 2024 15:21
@github-actions github-actions bot added the ui label Dec 6, 2024
Copy link
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-12-06 at 9 27 16 AM
I'm assuming we don't care about any sort of responsiveness (haven't checked with the UX team yet), but we should at least preserve some sort of padding for the text/image on the right side

Comment on lines 117 to 121
<P>
TAG serves as a powerful tool for both technical and security
teams to maintain robust access control structures, ensuring
security, compliance, and operational efficiency.
</P>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the name just Teleport Policy now and not TAG? Also, if it is still TAG, should we instead use the full name Teleport Access Graph for those not in the know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, @zmb3 already pointed it out. The copy turned out to be outdated. I'll update it.

<Flex>
<Box flex="1" width="min-content">
<P3>
TAG serves as a powerful tool for both technical and
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment on TAG

@bl-nero
Copy link
Contributor Author

bl-nero commented Dec 9, 2024

I'm assuming we don't care about any sort of responsiveness (haven't checked with the UX team yet), but we should at least preserve some sort of padding for the text/image on the right side

I'm adding paddings, but I have genuinely no idea how to make it properly react to the size here. The logic behind the layout should be "the promo box should have the width of the image, unless there is no room, then make it smaller, but don't collapse it to 0 while loading, and don't extend the container past the image size even if the text elements want to do it". If you know how to express it, please let me know. :)

@avatus
Copy link
Contributor

avatus commented Dec 9, 2024

I'm adding paddings,

i think just adding paddings is fine here tbh

@bl-nero bl-nero added the no-changelog Indicates that a PR does not require a changelog entry label Dec 9, 2024
@bl-nero bl-nero requested review from ravicious and avatus December 9, 2024 17:55
// to play well with the StepSlider below. Trying to use implicit
// width in combination with `width: min-content` inside a
// StepSlider child caused a miscalculation of child's height.
width={promoImageWidth + 2 * 2}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we just use +4 in that case?

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 went with "2*2", because I hoped it makes it more clear that I'm adjusting for 2 borders of 2 pixels.

Base automatically changed from bl-nero/role-editor-9 to master December 10, 2024 09:57
Comment on lines 140 to 144
// I found hardcoding the width of the container to be the only way
// to play well with the StepSlider below. Trying to use implicit
// width in combination with `width: min-content` inside a
// StepSlider child caused a miscalculation of child's height.
width={promoImageWidth + 2 * 2}
Copy link
Member

@ravicious ravicious Dec 10, 2024

Choose a reason for hiding this comment

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

Regarding responsiveness, here's something you could try:

Patch

Copy then pbpaste | git apply

diff --git a/web/packages/teleport/src/Roles/RoleEditor/RoleEditorAdapter.tsx b/web/packages/teleport/src/Roles/RoleEditor/RoleEditorAdapter.tsx
index 4b32c3e04b..ff05755e2d 100644
--- a/web/packages/teleport/src/Roles/RoleEditor/RoleEditorAdapter.tsx
+++ b/web/packages/teleport/src/Roles/RoleEditor/RoleEditorAdapter.tsx
@@ -109,17 +109,17 @@ export function RoleEditorAdapter({
         )}
       </Flex>
       <Flex flex="1" alignItems="center" justifyContent="center" m={3}>
-        <Box>
+        <Box maxWidth={782} minWidth={300}>
           <H1 mb={2}>Teleport Policy</H1>
-          <Flex mb={4}>
-            <Box flex="1" width="min-content">
+          <Flex mb={4} gap={4} flexWrap="wrap" justifyContent="space-between">
+            <Box flex="1" minWidth="30ch">
               <P>
                 Teleport Policy will visualize resource access paths as you
                 create and edit roles so you can always see what you are
                 granting before you push a role into production.
               </P>
             </Box>
-            <Flex flex="0 0 auto" ml={6} alignItems="start">
+            <Flex flex="0 0 auto" alignItems="start">
               <ButtonLockedFeature noIcon py={0} width={undefined}>
                 Contact Sales
               </ButtonLockedFeature>
@@ -137,24 +137,13 @@ export function RoleEditorAdapter({
             flexDirection="column"
             bg={theme.colors.levels.surface}
             borderRadius={3}
-            // I found hardcoding the width of the container to be the only way
-            // to play well with the StepSlider below. Trying to use implicit
-            // width in combination with `width: min-content` inside a
-            // StepSlider child caused a miscalculation of child's height.
-            width={promoImageWidth + 2 * 2}
           >
             <Box
               border={2}
               borderRadius={3}
               borderColor={theme.colors.interactive.tonal.neutral[0]}
             >
-              {/* Note: image dimensions hardcoded to prevent UI glitches while
-                  loading. */}
-              <Image
-                src={tagpromo}
-                width={promoImageWidth}
-                height={promoImageHeight}
-              />
+              <Image src={tagpromo} width="100%" />
             </Box>
             <StepSlider flows={promoFlows} currFlow="default" />
           </Flex>
@@ -164,9 +153,6 @@ export function RoleEditorAdapter({
   );
 }
 
-const promoImageWidth = 782;
-const promoImageHeight = 401;
-
 const promoFlows = {
   default: [PromoPanel1, PromoPanel2],
 };

The key is setting some max width for the entire box, so that the text doesn't just take all possible width.

I'm not 100% sure if this satisfies all your requirements, but it looks good enough IMHO.

loading-image.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course! It seems so obvious now. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

I was actually surprised that we don't have default responsive styles for <img>. But I suppose we don't use images that often.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was supposed to be width: 100%; height: auto, but in this case it doesn't matter because of that max-width on the parent.

https://stackoverflow.com/questions/15458650/make-an-image-responsive-the-simplest-way

@public-teleport-github-review-bot

@bl-nero - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

@bl-nero bl-nero enabled auto-merge December 10, 2024 15:48
@bl-nero bl-nero added this pull request to the merge queue Dec 10, 2024
Merged via the queue into master with commit 7939dcb Dec 10, 2024
41 checks passed
@bl-nero bl-nero deleted the bl-nero/role-editor-fullscreen branch December 10, 2024 16:05
@public-teleport-github-review-bot

@bl-nero See the table below for backport results.

Branch Result
branch/v17 Failed

bl-nero added a commit that referenced this pull request Dec 12, 2024
* Add admin rule tab to the role editor

Also tightens some constraints about standard role editor conformance.

* Add a way to delete an admin rule

* Add the options panel to role editor

* Move RoleEditorAdapter to a separate file

* Move the role editor to a full-screen dialog

* Review

* Review

* Review

* Lint

* Review
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
* Add admin rule tab to the role editor

Also tightens some constraints about standard role editor conformance.

* Add a way to delete an admin rule

* Add the options panel to role editor

* Move RoleEditorAdapter to a separate file

* Move the role editor to a full-screen dialog

* Review

* Review

* Review

* Lint

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

Successfully merging this pull request may close these issues.

3 participants