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

fixing string placement in opensearch_dashboards_overview #3670

Closed
wants to merge 11 commits into from

Conversation

mukatay1
Copy link

@mukatay1 mukatay1 commented Mar 23, 2023

Description

The goal of this task is to enhance the readability and maintainability of the Overview component in the OpenSearch Dashboards project. By completing this task, the Overview component will be more maintainable, easier to understand, and adhere to the project's coding standards. This will facilitate future development and reduce the likelihood of introducing bugs or regressions.

Issues Resolved

Improved readability and maintainability of the Overview component
#3049

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

@mukatay1 mukatay1 requested a review from a team as a code owner March 23, 2023 10:57
@abbyhu2000 abbyhu2000 self-assigned this Mar 23, 2023
@abbyhu2000 abbyhu2000 added OSCI Open Source Contributor Initiative branding labels Mar 23, 2023
@ashwin-pc
Copy link
Member

@mukatay1 can you fix the lint errors in the code and add a changelog entry for the change?

@mukatay1
Copy link
Author

can you fix the lint errors in the code and add a changelog entry for the change?

Hi, yes I can.

@mukatay1
Copy link
Author

@mukatay1 can you fix the lint errors in the code and add a changelog entry for the change?

@ashwin-pc @abbyhu2000 Can you please review this pr?

@@ -206,6 +206,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multi DataSource] UX enhancement on Data source management stack ([#2521](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2521))
- [Multi DataSource] UX enhancement on Update stored password modal for Data source management stack ([#2532](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2532))
- [Monaco editor] Add json worker support ([#3424](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3424))
- Fixing string placement in opensearch_dashboards_overview ([#3670](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3670))
Copy link
Member

Choose a reason for hiding this comment

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

minor style nits - we prefer the imperative "use" or "fix" over "using" or "fixing". I also updated to be more reflective of the new feature.

Suggested change
- Fixing string placement in opensearch_dashboards_overview ([#3670](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3670))
- [Overview] Use branding.applicationTitle as page header ([#3670](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3670))

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your feedback and updates, I'll make sure to use the imperative form and keep the new feature in mind for future responses.

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2023

Codecov Report

Merging #3670 (5b42496) into main (53047b6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3670      +/-   ##
==========================================
- Coverage   66.45%   66.44%   -0.01%     
==========================================
  Files        3209     3209              
  Lines       61633    61634       +1     
  Branches     9506     9507       +1     
==========================================
- Hits        40960    40955       -5     
- Misses      18396    18400       +4     
- Partials     2277     2279       +2     
Flag Coverage Δ
Linux 66.39% <100.00%> (-0.01%) ⬇️
Windows 66.40% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...s_overview/public/components/overview/overview.tsx 58.69% <100.00%> (+0.91%) ⬆️

... and 3 files with indirect coverage changes

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

@abbyhu2000 abbyhu2000 requested a review from kavilla March 29, 2023 18:48
Copy link
Member

@abbyhu2000 abbyhu2000 left a comment

Choose a reason for hiding this comment

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

I believe we can just utilize the <DefaultMessage> like below. The default is branding.applicationTitle is alreadyOpenSearch Dashboards

title={
          <FormattedMessage
            defaultMessage={branding.applicationTitle}
            id="opensearchDashboardsOverview.header.title"
          />
        }

defaultMessage="OpenSearch Dashboards"
id="opensearchDashboardsOverview.header.title"
/>
)
Copy link
Member

@abbyhu2000 abbyhu2000 Apr 5, 2023

Choose a reason for hiding this comment

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

@mukatay1 What do you think of the above suggestion? It is aligned with previous application title configuration for the welcome title:

defaultMessage={`Welcome to ${branding.applicationTitle}`}

Copy link
Author

Choose a reason for hiding this comment

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

Its sounds great

@abbyhu2000
Copy link
Member

@mukatay1 Do you think you are able to make changes to the PR based on the suggestions and also rebase this PR?

@joshuarrrr
Copy link
Member

@mukatay1 I'm converting this PR to a draft for now - when you have a chance to work on it again, you can take it out of draft mode.

@joshuarrrr joshuarrrr added the needs more info Requires more information from poster label May 12, 2023
@joshuarrrr joshuarrrr marked this pull request as draft May 12, 2023 22:23
@mukatay1 mukatay1 closed this May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branding needs more info Requires more information from poster OSCI Open Source Contributor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants