Skip to content

[ui-importer-modal] Bulk Edit column modal second iteration #4165

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

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

Conversation

ananya-agarwal
Copy link
Collaborator

@ananya-agarwal ananya-agarwal commented Jun 10, 2025

What changes were proposed in this pull request?

Adding edit columns button on the right and on clicking it, getting the modal with 4 different columns, a cancel and a done button, where the user will be able to edit the column header from the modal and on clicking Done button, he/she will be able to see the updated column header (name) in the preview. Also, getting pre-filled data from the file (say csv) into the modal.

How was this patch tested?

Manual testing, will add unit tests soon.

Video of what is done in the PR:

Screen.Recording.2025-07-14.at.3.14.38.PM.mov

Please review Hue Contributing Guide before opening a pull request.

Copy link

github-actions bot commented Jun 10, 2025

✅ Test files were modified. Ensure that the tests cover all relevant changes. ✅

Copilot

This comment was marked as outdated.

Copy link

github-actions bot commented Jun 10, 2025

UI Code Coverage Report

Lines Statements Branches Functions
Coverage: 33%
39.48% (30849/78129) 31.21% (14368/46034) 24.89% (2234/8973)

@cloudera cloudera deleted a comment from Copilot AI Jun 10, 2025
@cloudera cloudera deleted a comment from Copilot AI Jun 10, 2025
Copy link

github-actions bot commented Jun 10, 2025

Coverage

Backend Code Coverage Report •
FileStmtsMissCoverMissing
TOTAL547382739849% 
report-only-changed-files is enabled. No files were changed during this commit :)

Copy link
Collaborator

@ramprasadagarwal ramprasadagarwal left a comment

Choose a reason for hiding this comment

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

Nice work! See comments below

@bjornalm
Copy link
Collaborator

Can we get some screenshots for this PR?

Copilot

This comment was marked as outdated.

@ananya-agarwal ananya-agarwal requested a review from Copilot July 7, 2025 08:13
Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

@ananya-agarwal ananya-agarwal requested a review from Copilot July 14, 2025 09:00
Copilot

This comment was marked as outdated.

@ananya-agarwal ananya-agarwal requested a review from Copilot July 14, 2025 09:45
Copilot

This comment was marked as outdated.

@ananya-agarwal ananya-agarwal requested a review from Copilot July 14, 2025 10:19
Copilot

This comment was marked as outdated.

@ananya-agarwal ananya-agarwal requested a review from Copilot July 16, 2025 08:55
Copilot

This comment was marked as outdated.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a bulk edit column modal for the UI importer, allowing users to edit column headers, types, and comments through a modal interface. The modal displays sample data to help users make informed decisions about column configurations.

  • Adds a fully functional edit columns modal with input fields for name, type selection, sample data display, and comment editing
  • Integrates the modal with the main file preview component, enabling column data to be updated and persisted
  • Includes comprehensive test coverage for the new modal functionality

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
ImporterFilePreview.tsx Integrates edit columns modal with state management for column data and destination configuration
ImporterFilePreview.scss Adds styling for the edit columns button
EditColumnsModal.tsx Implements the complete modal with table, input fields, type selection, and data handling
EditColumnsModal.test.tsx Adds comprehensive unit tests for the modal functionality
EditColumnsModal.scss Defines styling for modal components including input and select elements
utils.ts Adds SQL type mapping API URL constant

@ananya-agarwal ananya-agarwal requested a review from Harshg999 July 28, 2025 10:25
@@ -14,10 +14,22 @@
// See the License for the specific language governing permissions and
// limitations under the License.

@use 'variables' as vars;
@import '../../../../components/styles/variables';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

width: 100px;
}

&__select--type {
Copy link
Collaborator

Choose a reason for hiding this comment

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

select--type is not a BEM best practice.
-- is used for css property like select--hidden or select--nowrap

];
const sample = { importerDataKey: 'row1', col1: 'val1', col2: 42 };

test('lists existing modal columns as expected', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We follow it('should pattern for starting the test case description.
Although test(' is also correct but for consistency lets stick to it('should

data: sqlTypesData,
loading: sqlTypesLoading,
error: sqlTypesError
} = useLoadData<string[]>(`${SQL_TYPE_MAPPING_API_URL}?sql_dialect=hive`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we hardcoding the sql_dialect to hive here?
Can we avoid it?
If not, then add a TODO to make it dynamic in future.

className="hue-importer-edit-columns-modal__select--type"
getPopupContainer={triggerNode => triggerNode.parentNode}
disabled={sqlTypesLoading || sqlTypes.length === 0}
loading={sqlTypesLoading}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the options prop available in the Select tag instead?

import { ImporterTableData, BaseColumnProperties } from '../../types';

export interface Column extends BaseColumnProperties {
title: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sticking to title or name?
In case we are sticking to title, then make sure everywhere else we have title.

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.

5 participants