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

[Discover] UI Updates to Dataset Configurator #8166

Merged
merged 10 commits into from
Oct 8, 2024

Conversation

sejli
Copy link
Member

@sejli sejli commented Sep 12, 2024

Description

  • Fixes blank row at bottom of Discover, thanks @ashwin-pc @AMoo-Miki for the change
  • Fixes an issue where indexes were able to be selected with a placeholder time field, which would cause issues
  • Disables time field selector in index pattern configurator if the index pattern already has a time field.

Issues Resolved

Screenshot

image image image

Testing the changes

Changelog

  • fix: Fixes UI issues in Discover and data configurator

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 6.25000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 60.94%. Comparing base (1f9da21) to head (542d89f).
Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
...s/data/public/ui/dataset_selector/configurator.tsx 0.00% 14 Missing ⚠️
.../data_explorer/public/components/app_container.tsx 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8166      +/-   ##
==========================================
- Coverage   60.95%   60.94%   -0.01%     
==========================================
  Files        3758     3759       +1     
  Lines       89288    89330      +42     
  Branches    13970    13977       +7     
==========================================
+ Hits        54422    54441      +19     
- Misses      31478    31491      +13     
- Partials     3388     3398      +10     
Flag Coverage Δ
Linux_1 28.92% <0.00%> (+0.02%) ⬆️
Linux_2 56.30% <ø> (-0.06%) ⬇️
Linux_3 37.78% <6.25%> (+<0.01%) ⬆️
Linux_4 29.93% <0.00%> (-0.02%) ⬇️
Windows_1 28.93% <0.00%> (+0.02%) ⬆️
Windows_2 56.25% <ø> (-0.06%) ⬇️
Windows_3 37.78% <6.25%> (-0.01%) ⬇️
Windows_4 29.93% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashwin-pc
Copy link
Member

Disables time field selector in index pattern configurator if the index pattern already has a time field.

The time field selector should be disabled no matter what for index patterns. So even if they have an index pattern where they have selected no time field, this should not let the user edit it.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Just lemme know if my functional logic makes sense.

@@ -97,7 +110,20 @@ export const Configurator = ({
>
<EuiFieldText disabled value={dataset.title} />
</EuiFormRow>
{timeFields && timeFields.length > 0 && (
{dataset.type === DEFAULT_DATA.SET_TYPES.INDEX_PATTERN &&
Copy link
Member

Choose a reason for hiding this comment

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

The logic here reads wrong. Correct me if my understanding is wrong but shouldnt the logic be like this:

if(languageService.getLanguage(language)?.disableDatePicker) {
  return null
}

if(dataset.type === DEFAULT_DATA.SET_TYPES.INDEX_PATTERN) {
  return DisabledInput
} else {
  return Select
}

@@ -100,6 +100,7 @@ export class QueryEnhancementsPlugin
editor: enhancedSQLQueryEditor,
editorSupportedAppNames: ['discover'],
supportedAppNames: ['discover', 'data-explorer'],
disableDatePicker: true,
Copy link
Member

Choose a reason for hiding this comment

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

+1

@ashwin-pc
Copy link
Member

lgtm! just a comment about the name of the config to ensure consistency. but if we wanted to do that we should do that as a fast follow and not block this pr.

also should we just with @opensearch-project/opensearch-ux . the language selector is below the timefield selection but the language will display or hide the the timefield selection so the language is shifting. should we consider a different order?

Yeah lets flip the order!

sejli pushed a commit to sejli/OpenSearch-Dashboards that referenced this pull request Oct 2, 2024
@sejli sejli force-pushed the index-pattern-time-field branch from 5ac9360 to 51e4490 Compare October 2, 2024 01:21
@sejli sejli force-pushed the index-pattern-time-field branch from 51e4490 to 57c3d12 Compare October 2, 2024 23:08
Signed-off-by: Sean Li <[email protected]>
joshuali925
joshuali925 previously approved these changes Oct 3, 2024
value: field.name,
})),
{ text: '-----', value: '-----', disabled: true },
{ text: noTimeFilter, value: noTimeFilter },
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 this is fine but it's better to separate them, since text can change (e.g. different translation, future UX wording update), and value should not change

Copy link
Member

Choose a reason for hiding this comment

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

+1

sejli added a commit to sejli/OpenSearch-Dashboards that referenced this pull request Oct 3, 2024
value: field.name,
})),
{ text: '-----', value: '-----', disabled: true },
{ text: noTimeFilter, value: noTimeFilter },
Copy link
Member

Choose a reason for hiding this comment

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

+1

@sejli sejli merged commit 3ae4cdc into opensearch-project:main Oct 8, 2024
66 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 8, 2024
* adding UI changes

Signed-off-by: Sean Li <[email protected]>

* addressing comments

Signed-off-by: Sean Li <[email protected]>

* fixing more bugs, adding no time field prop for unsupported languages

Signed-off-by: Sean Li <[email protected]>

* addressing comments

Signed-off-by: Sean Li <[email protected]>

* addressing comments

Signed-off-by: Sean Li <[email protected]>

* Changeset file for PR #8166 created/updated

* adressing comments, renaming label

Signed-off-by: Sean Li <[email protected]>

* addressing comments, adding tests

Signed-off-by: Sean Li <[email protected]>

* updating snapshots

Signed-off-by: Sean Li <[email protected]>

* adding i18n translation for no time filter option

Signed-off-by: Sean Li <[email protected]>

---------

Signed-off-by: Sean Li <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 3ae4cdc)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sejli pushed a commit that referenced this pull request Oct 9, 2024
* adding UI changes



* addressing comments



* fixing more bugs, adding no time field prop for unsupported languages



* addressing comments



* addressing comments



* Changeset file for PR #8166 created/updated

* adressing comments, renaming label



* addressing comments, adding tests



* updating snapshots



* adding i18n translation for no time filter option



---------



(cherry picked from commit 3ae4cdc)

Signed-off-by: Sean Li <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants