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

Fixed the issue where long values overflowed the modal in the pages u… #8640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Annaseli
Copy link
Contributor

…nder the "Administration" tab in the UI.

Closes #8489

Issue

Long values overflowed modal windows across multiple pages in the "Administration" tab. Initially reported for one modal, but found in Users, Groups, and other sections (for other sections I opened another issue).

Fix

  • Applied truncation for long values in modals.
  • Users can hover to see full value and copy the truncated value.
  • Fix applied globally to relevant components where was possibly.

Changes Made

1. "Add to Group" Pop-up (Groups -> click on some group name -> Add Members)

Updated the DataTable component to prevent overflow.

Changed in files:

webui/src/lib/components/controls.jsx (DataTable)
webui/src/lib/components/auth/forms.jsx (AttachModal)

The issue screenshot:

Add_to_Group_issue

Fix:

Add_to_Group_fix

Note

This pop-up is implemented using a "DataTable" and AttachModal, which is also used in other functions.

For example it fixed in addition this issue:
Issue:
long_group_name_issue_1
long_group_name_issue_2

Fix:
long_group_name_fix

2. "Groups" Page (Groups -> Group Name) "Users" Page (Users -> User Name)

Fixed GroupHeader, PolicyHeader, UserHeader overflow.

Changed in files:

webui/src/lib/components/auth/nav.jsx (GroupHeader, PolicyHeader, UserHeader)

  • Chaned in PolicyHeader since it has the same structure as GroupHeader and UserHeader

The issue screenshot:

long_name_title_issue_1

Fix:

long_name_title_fix

3. Fixed user name overflow in confirmation pop-up (Users -> selecting a user with a long name -> Access Credentials -> Create Access Key)

Changed in files:

webui/src/pages/auth/users/user/credentials.jsx (getMsg function)

The issue screenshot:

long_user_name_in_confirm_issue

Fix:

long_user_name_in_confirm_fix

Testing

Verified on lakeFS OSS using local DB & object store.

@Annaseli Annaseli requested review from guy-har and itaigilo February 11, 2025 17:37
@Annaseli Annaseli added the include-changelog PR description should be included in next release changelog label Feb 11, 2025
Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

14 passed

Copy link

E2E Test Results - Quickstart

11 passed

Copy link
Contributor

@guy-har guy-har left a comment

Choose a reason for hiding this comment

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

Looks Amazing!
Great work!
requesting changes because the checkbox has too much place now

image

@@ -9,6 +9,7 @@ import {SearchIcon} from "@primer/octicons-react";
import {useAPI} from "../../hooks/api";
import {Checkbox, DataTable, DebouncedFormControl, AlertError, Loading} from "../controls";

import "../../../styles/globals.css"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required? IIUC it's importet in main.tsx which should be enough...
This comment goes for all other files where this import exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right it works without the import

Copy link
Contributor

@itaigilo itaigilo left a comment

Choose a reason for hiding this comment

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

Great first big PR -
Really nice effort mapping the issues, looking around, and getting to a complete solution.

Some notes:

  1. Agree with @guy-har 's comment about limiting the width of the checkbox.
  2. You should consider splitting this PR into one with the specific changes, and one with the general ones, that will apply multiple components. Since changing global components (potentially) affects the whole app, I think it's better to have a separate PR and discussion for it.
  3. Nit: better to have a shorter title for the PR, for better readability. 😊

Anyway, nice work 💪

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking, we should try to avoid using globals.css as much as possible.
Historically it's been used quite heavily, but there's a task to change this.
The reasoning behind it is to let the bootstrap framework do all the UI work for us, and "guide" it using bootstrap classes. When we're using global classes, it's easy to fix one thing while breaking another one. Using the bootstrap classes gives us more confidence that the maintainers of the framework already checked this on different components.
I would start with the table component, and see which of these you can utilize.

Anyway, classes should be added to globals.css only as a last resort.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although it's tempting, please try to avoid re-formatting the whole file (unless you're changing big parts of it).
It makes reviewing harder, and messes with the git history.

There is a plan to apply auto-formatting on the React code, but it's not applied yet.

@@ -43,7 +44,18 @@ const UserCredentialsList = ({ userId, after, onPaginate }) => {
</>
);

const getMsg = (email) => <span>Create new credentials for user <strong>{email}</strong>?</span>;
const getMsg = (email) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: records with long names overflow modals boundaries
3 participants