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

chore(frontend): Spelling #25452

Merged
merged 62 commits into from
Sep 10, 2024
Merged

chore(frontend): Spelling #25452

merged 62 commits into from
Sep 10, 2024

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Sep 28, 2023

SUMMARY

Spelling fixes for the frontend

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Comment on lines -208 to +209
* Collapes Native Filter from the left panel on dashboard
* Collapses Native Filter from the left panel on dashboard
* @returns {None}
* @summary helper for collape native filter
* @summary helper for collapse native filter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling we talked about this in the past, but I'm not certain...

program_language: 'Javscript',
program_language: 'JavaScript',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

similarly.

fwiw, I threw out my baseline because I wanted to test the next version of check-spelling...

// When clicking a subdivision, the vis will zoom in to it
// When clicking a subdivision, the vis will zoom into it
Copy link
Contributor Author

Choose a reason for hiding this comment

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

often controversial

// TODO the test bellow is flaky, and has been disabled for the time being
// TODO the test below is flaky, and has been disabled for the time being
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it('should assign allowed non-existent value', () => {
it('should assign allowed nonexistent value', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

often controversial (I can't find the discussion)

search: '?unkown_param=value',
search: '?unknown_param=value',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

always unclear when tests do things like this what they're testing...

Comment on lines -182 to +191
const isGranularitySqalTemporal = columns.find(
const isGranularitySqlaTemporal = columns.find(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

brand

Comment on lines -200 to +199
.find('#duplicate-action-tooltop')
.find('#duplicate-action-tooltip')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

notable

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you got it in the other location where it's set, so we should be fine.

// setup appwide custom error messages
// set up app wide custom error messages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

notable. setup is a noun but this wants the verb-phrase, and there's no particularly good reason to merge app+wide

Copy link
Member

Choose a reason for hiding this comment

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

arguably it should be app-wide.

Copy link
Member

Choose a reason for hiding this comment

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

... but I won't lose any sleep over that.

"Don't use variables in translation string templates. Flask-babel is a static translation translation service, so it can’t handle strings that include variables",
"Don't use variables in translation string templates. Flask-babel is a static translation service, so it can’t handle strings that include variables",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

duplicate words are usually typos...

@jsoref jsoref force-pushed the spelling-frontend branch 3 times, most recently from f0834ea to 4c64aa8 Compare September 28, 2023 23:35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, these files are a mix of frontend/backend. I should have looked more carefully...

Comment on lines -228 to +232
function openMoreFilters(intercetFilterState = true) {
function openMoreFilters(waitFilterState = true) {
interceptFilterState();
cy.getBySel('dropdown-container-btn').click();

if (intercetFilterState) {
if (waitFilterState) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This typo meant that people didn't realize that they were almost shadowing a function. I've renamed the argument to wait instead of intercept to more accurately describe what the boolean guards...

@@ -170,7 +170,7 @@ export const showControls: CustomControlItem = {
default: false,
description: t(
'Whether to show extra controls or not. Extra controls ' +
'include things like making mulitBar charts stacked ' +
'include things like making multiBar charts stacked ' +
Copy link
Member

Choose a reason for hiding this comment

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

this might actually need to be multi-bar... but what you have here is an improvement, so that's fine :D

@@ -493,7 +493,7 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
)}
{canDuplicate && original.kind === 'virtual' && (
<Tooltip
id="duplicate-action-tooltop"
id="duplicate-action-tooltip"
Copy link
Member

Choose a reason for hiding this comment

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

here's the instance you caught (thanks!) that affects the script elsewhere.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM! There's a few things we could nit-pick, perhaps, but this is a ton of nice fixes... let's merge it when CI passes!

@rusackas
Copy link
Member

Codeowner stamp needed from @kgabryje @geido or @michael-s-molina

@rusackas
Copy link
Member

Oof... not so sure about this one @jsoref - looks like a more painful rebase situation than the other PR :D

@jsoref
Copy link
Contributor Author

jsoref commented Aug 20, 2024

lemme look...

jsoref added 8 commits August 20, 2024 16:55
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
jsoref added 19 commits August 20, 2024 17:01
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@jsoref jsoref force-pushed the spelling-frontend branch from 2b7a758 to 475ff79 Compare August 20, 2024 21:02
@github-actions github-actions bot added i18n Namespace | Anything related to localization plugins packages labels Aug 20, 2024
@jsoref
Copy link
Contributor Author

jsoref commented Aug 20, 2024

@rusackas: rebased -- thankfully single correction scoped commits isn't as daunting as a squashed quagmire when rebasing :)

@rusackas rusackas merged commit d0c9cde into apache:master Sep 10, 2024
34 checks passed
@jsoref jsoref deleted the spelling-frontend branch September 10, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n Namespace | Anything related to localization packages plugins size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants