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

[CSV Upload] Throw warning when a column name contains spaces #7995

Closed
wants to merge 4 commits into from

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Aug 13, 2016

When importing a CSV via the new CSV importer column names have been used as printed in CSV.

Since field names with space are problematic to use in Kibana, the importer now shows a warning when the CSV contains any fields with spaces in their names.

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@Bargs Bargs self-assigned this Aug 15, 2016
@@ -72,6 +72,10 @@ modules.get('apps/management')
this.formattedErrors.push('Column names must be unique');
}

if (results.meta.fields.some(name => /\s/.test(name))) {
this.formattedWarnings.push('Field names with spaces are dificult to search for in Kibana');
Copy link
Contributor

Choose a reason for hiding this comment

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

dificult difficult

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How the hell could that happen, I copied your sentence from IRC :D

Anyway... fixed it.

@Bargs
Copy link
Contributor

Bargs commented Aug 15, 2016

LGTM. @BigFunger could you take a second quick look at this?

@Bargs Bargs changed the title Convert CSV column names with spaces to camel case [CSV Upload] Throw warning when a column name contains spaces Aug 15, 2016
@Bargs
Copy link
Contributor

Bargs commented Aug 15, 2016

@timroes could you update the PR description to reflect the new design, just so no one's confused in the future?

@Bargs Bargs assigned BigFunger and unassigned Bargs Aug 15, 2016
@Bargs
Copy link
Contributor

Bargs commented Aug 16, 2016

@timroes actually... now that I think about it, would you mind waiting for #7950 to get merged? It would be nice for this new warning to get integrated into the new validateHeaders module that's being added, and then it could also report the column positions where there are spaces in names.

@Bargs Bargs assigned Bargs and unassigned BigFunger Aug 16, 2016
@timroes
Copy link
Contributor Author

timroes commented Aug 16, 2016

@Bargs no problem, we can wait for this to be merged. I would close this PR, because the modification doesn't have a lot to do with what was going on here earlier. So when #7950 is merged I will just open a fresh pull request and just reference this one for history.

@timroes timroes closed this Aug 16, 2016
@Bargs
Copy link
Contributor

Bargs commented Aug 16, 2016

@timroes fantastic, thanks

@Bargs
Copy link
Contributor

Bargs commented Aug 17, 2016

@timroes FYI #7950 is merged now

@timroes
Copy link
Contributor Author

timroes commented Aug 20, 2016

@Bargs Looking at the validateHeaders function I wouldn't add it to this, since this is really made for errors and not warnings. So I would still "just" add it to the parse_csv_step.js directly like it was before. Which therefore still wouldn't hold the column index of the field with spaces.

Other option would be to modify the validateHeaders function to be more about "issues" and not "errors", i.e. renaming all of the vars/tests/etc. in there.

Your opinion on this?

@Bargs
Copy link
Contributor

Bargs commented Aug 22, 2016

I think it'd be fine to update validateHeaders to check for both errors and warnings. It could probably be updated to return an object with top level errors and warnings keys, so that the calling code knows how to treat each one.

jbudz pushed a commit that referenced this pull request Sep 10, 2024
`v95.9.0`⏩`v95.10.1`

> [!note]
> **EuiDataGrid**'s header cells have received a major UX change in
order to support interactive children within header content. Column
header actions now must be hovered and then clicked directly, or opened
with the Enter key, as opposed to being able to click the entire header
cell to see the actions popover.

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v95.10.0`](https://github.com/elastic/eui/releases/v95.10.0)

- Updated `EuiDataGrid` to support interactive header cell content
([#7898](elastic/eui#7898))
- Updated `EuiSearchBar`'s `field_value_selection` filter type with a
new `autoSortOptions` config, allowing consumers to configure whether or
not selected options are automatically sorted to the top of the filter
list ([#7958](elastic/eui#7958))
- Updated `getDefaultEuiMarkdownPlugins` to support the following new
default plugin configurations:
([#7985](elastic/eui#7985))
- `parsingConfig.linkValidator`, which allows configuring
`allowRelative` and `allowProtocols`
  - `parsingConfig.emoji`, which allows configuring emoticon parsing
- `processingConfig.linkProps`, which allows configuring rendered links
with any props that `EuiLink` accepts
- See our **Markdown plugins** documentation for example
`EuiMarkdownFormat` and `EuiMarkdownEditor` usage
- Updated `EuiDatePicker` to support `append` and `prepend` nodes in its
form control layout ([#7987](elastic/eui#7987))

**Bug fixes**

- Fixed border rendering bug with inline `EuiDatePicker`s with
`shadow={false}` ([#7987](elastic/eui#7987))
- Fixed `EuiSuperSelect`'s placeholder text color to match other form
controls ([#7995](elastic/eui#7995))

**Accessibility**

- Improved the keyboard navigation and screen reader output for
`EuiDataGrid` header cells
([#7898](elastic/eui#7898))

## [`v95.10.1`](https://github.com/elastic/eui/releases/v95.10.1)

**Bug fixes**

- Fixed a visual bug in compact density `EuiDataGrid`s, where the header
cell height would increase when the actions button became visible
([#7999](elastic/eui#7999))

---------

Co-authored-by: Lene Gadewoll <[email protected]>
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