Skip to content

Conversation

brack2891
Copy link
Contributor

Addresses


https://broadworkbench.atlassian.net/browse/DT-2039

Summary


Adding proper clarity to speak to text option within: Research Console, Data Library, and Profile


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@brack2891 brack2891 requested a review from a team as a code owner September 16, 2025 03:16
@brack2891 brack2891 requested review from eweitz and rushtong and removed request for a team September 16, 2025 03:16
Copy link
Member

@eweitz eweitz left a comment

Choose a reason for hiding this comment

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

Kudos on your first DUOS PR!

Code looks generally good, but the duplicate config.json and linting issues seem worth fixing. FWIW, I often run npx eslint before pushing to surface formatting issues locally.

Copy link
Contributor

@rushtong rushtong 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! Let us know if you need help addressing the lint warnings.

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. I do see that some of the original feedback has not been applied yet:

  1. Changes to base_config.json should be reverted
  2. PaginatorBar.jsx should be deleted

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

See comment inline. It is preferable not to change the layout in any way.

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

See below:

@rushtong
Copy link
Contributor

rushtong commented Oct 3, 2025

This PR seems to have been reduced in scope significantly to the point that removing PaginatorBar.jsx is pretty unrelated to the ticket and the PR. I would suggest moving this file deletion to a separate PR which will help us keep features and PRs tightly coupled.

@brack2891
Copy link
Contributor Author

@rushtong just so I am clear, you'd like a separate PR for deleting PaginatorBar.jsx. Adding that file back in this PR so that it is only captured in the future PR? Am I following that correctly?

Copy link
Member

@eweitz eweitz left a comment

Choose a reason for hiding this comment

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

@rushtong, Austin has a question on how to proceed.

Please re-request review when this is ready for another round -- happy to take a look!

@rushtong
Copy link
Contributor

rushtong commented Oct 7, 2025

@rushtong, Austin has a question on how to proceed.

Please re-request review when this is ready for another round -- happy to take a look!

Thanks for the reminder, I'll take another look.

@rushtong
Copy link
Contributor

rushtong commented Oct 7, 2025

@rushtong just so I am clear, you'd like a separate PR for deleting PaginatorBar.jsx. Adding that file back in this PR so that it is only captured in the future PR? Am I following that correctly?

@brack2891 - Yes, exactly. In general, the PR should be focused on the required changes and that file deletion is essentially unrelated. By tying PRs to tickets, we can better track why something happened.

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

See below:

@brack2891 brack2891 requested review from fboulnois October 9, 2025 14:03
@brack2891 brack2891 force-pushed the ae-dt-2039-associated-labels branch from 380d638 to 3e41375 Compare October 10, 2025 19:04
@sonarqubecloud
Copy link

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't yet have an e2e github action so these tests are not run on PRs. When I run this locally, there are a number of failing tests. How did you get these to succeed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing some failures in this test as well when running locally.

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing failing tests

Copy link
Contributor

Choose a reason for hiding this comment

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

This file isn't used by any other components so it is essentially untestable. Changes here should be reverted and the file removed in a separate cleanup PR.

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.

4 participants