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

[MD] Datasource management Edit/Update page UX updates #2629

Merged
merged 5 commits into from
Oct 20, 2022

Conversation

mpabba3003
Copy link
Contributor

@mpabba3003 mpabba3003 commented Oct 19, 2022

Signed-off-by: mpabba3003 amazonmanideep@gmail.com

Description

Edit Data Source page

Issues Resolved

#2621
#2622

SCREENSHOTS

Data.Sources.-.OpenSearch.Dashboards.2.mp4

Check List

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

Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
@mpabba3003 mpabba3003 requested a review from a team as a code owner October 19, 2022 23:02
@mpabba3003 mpabba3003 self-assigned this Oct 19, 2022
@mpabba3003 mpabba3003 added multiple datasource multiple datasource project ux / ui Improvements or additions to user experience, flows, components, UI elements backport 2.x v2.4.0 'Issues and PRs related to version v2.4.0' labels Oct 19, 2022
Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #2629 (0d945ce) into main (33aed0a) will decrease coverage by 0.00%.
The diff coverage is 81.81%.

❗ Current head 0d945ce differs from pull request most recent head 7d335c3. Consider uploading reports for the commit 7d335c3 to get more accurate results

@@            Coverage Diff             @@
##             main    #2629      +/-   ##
==========================================
- Coverage   66.81%   66.80%   -0.01%     
==========================================
  Files        3207     3207              
  Lines       61137    61146       +9     
  Branches     9313     9314       +1     
==========================================
+ Hits        40849    40850       +1     
- Misses      18057    18063       +6     
- Partials     2231     2233       +2     
Impacted Files Coverage Δ
src/plugins/data_source_management/public/types.ts 100.00% <ø> (ø)
...c/components/edit_data_source/edit_data_source.tsx 87.75% <80.00%> (-4.09%) ⬇️
...rce/components/edit_form/edit_data_source_form.tsx 91.45% <84.61%> (-1.14%) ⬇️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 50.00% <0.00%> (-2.78%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zhongnansu
Copy link
Member

Probably update PR title to something meaningful, e.g DataSource update page refactor ? @mpabba3003

@mpabba3003 mpabba3003 changed the title [MD] #2622 & #2621 changes [MD] Datasource management Edit/Update page UX updates Oct 20, 2022
@mpabba3003
Copy link
Contributor Author

Probably update PR title to something meaningful, e.g DataSource update page refactor ? @mpabba3003

Done

zengyan-amazon
zengyan-amazon previously approved these changes Oct 20, 2022
Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
/>
</h4>
}
<EuiFormRow
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the snapshot tests for components like this? This diff does not make sense for the corresponding change in the test file and is a symptom of a bad snapshot. If we want to test the functionality of the component or its UI, we should leverage the functional test repo. Unit tests should only test the logic. the same comment applies to edit_data_source.

You need to test your components as a unit and in isolation, if one react component output changes only one unit test fails.

This principle breaks here because if EUI changes the markup for this component, all these tests will break.

@zhongnansu zhongnansu merged commit e056e83 into opensearch-project:main Oct 20, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 20, 2022
Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
(cherry picked from commit e056e83)
zhongnansu pushed a commit that referenced this pull request Oct 21, 2022
Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
(cherry picked from commit e056e83)

Co-authored-by: Manideep Pabba <109986843+mpabba3003@users.noreply.github.com>
@AMoo-Miki AMoo-Miki added the enhancement New feature or request label Nov 5, 2022
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
…oject#2629)

Signed-off-by: mpabba3003 <amazonmanideep@gmail.com>
Signed-off-by: Sergey V. Osipov <sipopo@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request multiple datasource multiple datasource project ux / ui Improvements or additions to user experience, flows, components, UI elements v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants