Skip to content

chore: Avoid utf-8 characters in code #5439

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 3, 2023
Merged

chore: Avoid utf-8 characters in code #5439

merged 1 commit into from
Feb 3, 2023

Conversation

Artur-
Copy link
Member

@Artur- Artur- commented Feb 3, 2023

Using utf-8 characters provide no benefit but can break file serving when you have encoding issues in your setup

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@tomivirkki tomivirkki 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 are more occurrences, however

@web-padawan web-padawan merged commit d32d0ce into master Feb 3, 2023
@web-padawan web-padawan deleted the lambda-fix branch February 3, 2023 15:49
web-padawan pushed a commit that referenced this pull request Feb 3, 2023
DiegoCardoso pushed a commit that referenced this pull request Feb 3, 2023
@Artur-
Copy link
Member Author

Artur- commented Feb 4, 2023

Ok so there are still a few others

packages/map/src/vaadin-map.js:319:          content: var(--vaadin-map-icon-close, '×');
packages/checkbox-group/src/vaadin-checkbox-group.d.ts:43: *   <vaadin-checkbox value="fr" label="Français"></vaadin-checkbox>
packages/checkbox-group/src/vaadin-checkbox-group.js:22: *   <vaadin-checkbox value="fr" label="Français"></vaadin-checkbox>
packages/menu-bar/src/vaadin-menu-bar-mixin.js:126:          dots.textContent = '···';

As they are actually used in the code, does it mean that you are assuming JS files should always be served as utf-8? It does not seem to be the case with Flow: vaadin/flow#15817

@web-padawan
Copy link
Member

Usage of these is not intentional. We need to replace these with entities (in case of map icon / menu-bar dots).

I'll also update checkbox group example in JSDoc comment and typings to not include non-English characters.

@Artur-
Copy link
Member Author

Artur- commented Feb 4, 2023

Btw these were found using grep -I --color='auto' -P -n "[\x80-\xFF]" -R packages|grep -v "/test/"

@Artur-
Copy link
Member Author

Artur- commented Feb 4, 2023

This seems to be related prettier/prettier#14293 as the unicode escape sequences are automatically removed by prettier on commit

@web-padawan
Copy link
Member

web-padawan commented Feb 4, 2023

Tried the above script via pcregrep and got this output. I'll create some PRs to get rid of these characters where possible.

packages/accordion/src/vaadin-accordion-heading.js:17: * convenient to use and doesn’t make styling of the heading more problematic.
packages/accordion/src/vaadin-accordion.js:15: * `<vaadin-accordion>` is a Web Component implementing accordion widget —
packages/avatar/src/vaadin-avatar.js:114:        <text dy=".35em" text-anchor="middle"></text>
packages/charts/src/vaadin-chart-series.js:140:       * Used to connect the series to an axis; if multiple series have the same “unit”, they will share axis.
packages/charts/src/vaadin-chart.js:190: *  <vaadin-chart title="۱- عنوان نمودار" subtitle="۲- عنوان فرعی نمودار"
packages/charts/src/vaadin-chart.js:192: *    "legend": {"rtl": true}, "yAxis": [{"id": "۴- مقادیر", "title": {"text": "۴- مقادیر", "useHTML": true}}],
packages/charts/src/vaadin-chart.js:196: *          title="۳- عنوان ردیف"
packages/charts/src/vaadin-chart.js:197: *          unit="۴- مقادیر"
packages/checkbox-group/src/vaadin-checkbox-group.js:22: *   <vaadin-checkbox value="fr" label="Français"></vaadin-checkbox>
packages/component-base/src/focus-mixin.js:42:        // input field on iOS after “Done” is pressed.
packages/component-base/src/focus-utils.js:255:  // the final array by tabindex.≈
packages/context-menu/src/vaadin-context-menu.js:97: * ### “vaadin-contextmenu” Gesture Event
packages/crud/theme/material/vaadin-crud-styles.js:30:      content: '✎';
packages/date-picker/theme/lumo/vaadin-date-picker-year-styles.js:28:      content: '•';
packages/field-base/src/styles/clear-button-styles.js:15:    content: '✕';
packages/form-layout/src/vaadin-form-layout.js:251:    // CSS values in `responsiveSteps`. We can’t add this to the `<template>`,
packages/form-layout/src/vaadin-form-layout.js:332:    // - `border-spacing` accepts `<length> | inherit`, it’s the best! But
packages/form-layout/src/vaadin-form-layout.js:427:      // Apply the chosen responsive step’s properties
packages/form-layout/src/vaadin-form-layout.js:484:          // Too big to fit on this row, let’s wrap it
packages/form-layout/theme/lumo/vaadin-form-item-styles.js:28:      content: var(--lumo-required-field-indicator, '•');
packages/grid/src/vaadin-grid-column-group.js:133:    // Don’t unfreeze the frozen group because of a non-frozen child
packages/grid/src/vaadin-grid-column-group.js:138:    // Don’t unfreeze the frozen group because of a non-frozen child
packages/grid/src/vaadin-grid-column-group.js:143:    // Don’t unfreeze the frozen group because of a non-frozen child
packages/grid/src/vaadin-grid-column-group.js:148:    // Don’t unfreeze the frozen group because of a non-frozen child
packages/grid/src/vaadin-grid-column-group.js:282:    // Don’t propagate the default `false` value.
packages/grid/src/vaadin-grid-column-group.js:298:    // Don’t propagate the default `false` value.
packages/grid/src/vaadin-grid-column-reordering-mixin.js:159:        // Reordering didn’t start. Skip this event.
packages/grid/src/vaadin-grid-column-reordering-mixin.js:199:        // Reordering didn’t start. Skip this event.
packages/grid/src/vaadin-grid-data-provider-mixin.js:465:            ' in the second argument of the `dataProvider`’s `callback` call.',
packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js:123:      // not escape the grid’s shadowRoot, thus listening inside the shadowRoot.
packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js:246:        // When in the interacting mode, only the “Interaction” keys are handled.
packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js:529:      // _focusedColumnOrder is memoized — this is to ensure predictable
packages/grid/src/vaadin-grid-keyboard-navigation-mixin.js:548:        // Find orderedColumnIndex — the index of order closest matching the
packages/grid/src/vaadin-grid-row-details-mixin.js:94:      // Skip to avoid duplicate work of both “.splices” and “.length” updates.
packages/grid/src/vaadin-grid.js:139: * and the “Assigning Data” page in the demos.
packages/grid/src/vaadin-grid.js:780:          // If focus is on element within the cell content — respect it, do not change
packages/map/src/vaadin-map.js:261:          content: var(--vaadin-map-icon-zoom-out, '–');
packages/map/src/vaadin-map.js:277:          content: var(--vaadin-map-icon-attribution-collapse, '▸');
packages/map/src/vaadin-map.js:281:          content: var(--vaadin-map-icon-attribution-expand, 'ℹ');
packages/map/src/vaadin-map.js:307:          content: var(--vaadin-map-icon-compass, '↑');
packages/map/src/vaadin-map.js:315:          content: var(--vaadin-map-icon-fullscreen, '⤢');
packages/map/src/vaadin-map.js:319:          content: var(--vaadin-map-icon-close, '×');
packages/map/src/vaadin-map.js:329:          content: var(--vaadin-map-icon-overview-map-collapse, '▾');
packages/map/src/vaadin-map.js:333:          content: var(--vaadin-map-icon-overview-map-expand, '▴');
packages/menu-bar/src/vaadin-menu-bar-mixin.js:126:          dots.textContent = '···';
packages/number-field/src/vaadin-number-field.js:66:          content: '−';
packages/overlay/src/vaadin-overlay.js:206:       * it doesn’t change the functionality of the user interface.
packages/rich-text-editor/src/vaadin-rich-text-editor-toolbar-styles.js:127:    content: '”';
packages/select/src/vaadin-select.js:252:       * When there’s an item selected, it's the value of that item, otherwise
packages/tabs/src/vaadin-tabs.js:113:          content: '◀';
packages/tabs/src/vaadin-tabs.js:117:          content: '▶';
packages/tabs/src/vaadin-tabs.js:128:          content: '▶';
packages/tabs/src/vaadin-tabs.js:132:          content: '◀';
packages/vaadin-lumo-styles/mixins/required-field.js:48:    content: var(--lumo-required-field-indicator, '•');
packages/vaadin-themable-mixin/vaadin-theme-property-mixin.js:19:         * the sub-component’s "theme" attribute to the `theme` property of

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.0.0.beta1 and is also targeting the upcoming stable 24.0.0 version.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants