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

[Styleguide] ES2015 classes #7361

Closed
wants to merge 1 commit into from
Closed

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jun 2, 2016

I guess I don't have enough conflict in my life, so I decided to start a discussion about classes.

I felt ambivalent about classes for awhile. At first they seemed like fairly benign syntactic sugar. I tried to give them a shot, and I regretted it. I think we should avoid them entirely for the time being. I jotted down a few reasons in the styleguide but I wanted to keep it concise, so I'll elaborate a bit here.

My #1 problem with classes is that they make encapsulation impossible. Not only do they not provide a way to create private members, they also prevent you from hiding private instance members inside a closure. To me, this is reason enough to avoid them. What's OOP without encapsulation?

Classes encourage inheritance. Older languages with classes learned long ago that its better to favor composition of inheritance. Inheritance is only really useful if you have a type system, and ES2015 classes aren't types.

It's generally a best practice to avoid the new keyword. Classes require use of the new keyword. The new keyword is rigid and ties callers to a specific type of object instantiation. Again, older languages with classes have learned to prefer factory functions over constructors because they're more flexible and more powerful.

What benefits do we get for all of these negatives? Slightly fewer characters? I might have said better static analysis, but my editor seems to parse old school constructor functions just as well as classes.

Some links for further discussion:

https://medium.com/javascript-scene/how-to-fix-the-es6-class-keyword-2d42bb3f4caf#.msy3xzais
http://christianalfoni.github.io/javascript/2015/01/01/think-twice-about-classes.html

@Bargs Bargs added the discuss label Jun 2, 2016
@cjcenizal
Copy link
Contributor

I completely agree with most of your supporting arguments:

  1. Prefer composition over inheritance.
  2. Use encapsulation effectively (to present a clear and simple interface).
  3. Avoid use of new when a factory method provides a more intuitive or flexible construction interface.

But I also don't think prohibiting the use of class will necessarily help people use encapsulation effectively, use the factory pattern effectively, or prefer composition over inheritance. If those are our goals, then I think our best shot is to list those bullets as desired traits in our codebase, with links to useful resources, and then refer to them in code review.

I also think a problem with prohibiting language features is that it risks becoming dogmatic... I've found it's easy to lose sight of the end goals with such broad-stroke rules, and it creates a barrier for great use-cases for the outlawed feature, which may be discovered by brilliant minds in the future.

@tsullivan
Copy link
Member

I'm with you here, especially in regards to the point you raised about lack of private members. Whenever I've started out using classes, I've always been happier refactoring the code to use an enclosure that keeps data private, and not use new and avoid this.

@Bargs Bargs closed this Nov 3, 2016
cee-chen added a commit that referenced this pull request Jan 5, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([#7392](elastic/eui#7392))

## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0)

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
`v91.0.0-backport.0`⏩`v91.3.1`

⚠️ The largest set of changes in this PR that touch source code (as
opposed to test code) are related to several **EuiDataGrid** redesigns,
particularly around the toolbar, column cell headers, and cell actions.
We **strongly** recommend QAing your EuiDataGrid usages, **especially**
if you have custom CSS styling on data grid cells.

| Changes | Screencap |
|--------|--------|
| Cell actions and popover | <img
src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0"
width="240" alt=""> |
| Column headers | <img
src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2"
alt="" width="360"> |
| Toolbar | <img
src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3"
alt="" width="240"> |

---

## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1)

**Bug fixes**

- Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton`
test subject attribute from to the clickable button, for easier E2E
testing ([elastic#7427](elastic/eui#7427))
- Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as
disabled when rows are being selected
([elastic#7428](elastic/eui#7428))

## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0)

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
([elastic#7399](elastic/eui#7399))
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include
entry indexes ([elastic#7406](elastic/eui#7406))

## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0)

- Added `endpoint` glyph to `EuiIcon`
([elastic#7383](elastic/eui#7383))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([elastic#7392](elastic/eui#7392))

## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0)

- Updated `EuiDataGrid` cell actions to display above cells instead of
within them, to avoid content clipping issues
([elastic#7343](elastic/eui#7343))
- Updated `EuiDataGrid` cell expansion popovers to sit on top of cells
instead of below/next to them
([elastic#7343](elastic/eui#7343))
- Updated `EuiListGroupItem` to render an external icon and screen
reader affordance for links with `target` set to to `_blank`
([elastic#7352](elastic/eui#7352))
- Updated `EuiListGroupItem` with a new `external` prop, which allows
enabling or disabling the new external link icon
([elastic#7352](elastic/eui#7352))
- Updated `EuiText` to no longer set any opinionated styles on child
`<img>` tags - use `EuiImage` for image display within text instead
([elastic#7360](elastic/eui#7360))
- Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom
actions ([elastic#7361](elastic/eui#7361))
- Added a new `EuiDataGridToolbarControl` subcomponent, which is useful
for rendering your own custom `EuiDataGrid` toolbar buttons while
matching the look of the default controls
([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid`'s toolbar controls to show active/current counts
in badges, and updated the Columns button icon
([elastic#7369](elastic/eui#7369))
- Updated `EuiButtonEmpty` to allow passing `false` to `textProps`,
which allows rendering custom button content without an extra text
wrapper ([elastic#7369](elastic/eui#7369))
- Updated `EuiDataGrid` column header cells to show the sort arrow after
the heading text, instead of before
([elastic#7371](elastic/eui#7371))
- Updated `EuiDataGrid`'s column header actions icon from a chevron to
`boxesVertical` ([elastic#7371](elastic/eui#7371))
- Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s.
Alongside `name`, the `description`, `href`, and `data-test-subj`
properties now also accept an optional callback that the current `item`
will be passed to ([elastic#7373](elastic/eui#7373))
- Updated `EuiContextMenuItem` with a new `toolTipProps` prop
([elastic#7373](elastic/eui#7373))
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([elastic#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([elastic#7388](elastic/eui#7388))

**Bug fixes**

- Fixed incorrect `EuiPopover` positioning calculations when `hasArrow`
was set to false ([elastic#7343](elastic/eui#7343))
- Fixed `EuiSuperSelect` to render options with falsy values (false, 0,
and ''), but not nullish values (undefined or null)
([elastic#7362](elastic/eui#7362))
- Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g.,
booleans or numbers) ([elastic#7362](elastic/eui#7362))
- Fixed `EuiDataGrid`'s numeric and currency column heading cells to be
correctly right-aligned
([elastic#7371](elastic/eui#7371))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing
tooltip descriptions when rendered in the all actions popover menu
([elastic#7373](elastic/eui#7373))
- Fixed missing underlines on `EuiContextMenu` link hover
([elastic#7373](elastic/eui#7373))
- Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent`
([elastic#7375](elastic/eui#7375))
- Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off
in vertical alignment
([elastic#7380](elastic/eui#7380))

**Deprecations**

- Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use
`toolTipProps.title` instead
([elastic#7373](elastic/eui#7373))
- Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use
`toolTipProps.position` instead
([elastic#7373](elastic/eui#7373))

**Accessibility**

- Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested
interactive custom actions
([elastic#7361](elastic/eui#7361))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly
reading out action descriptions to screen readers
([elastic#7373](elastic/eui#7373))
- Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not
visibly appearing on keyboard focus
([elastic#7373](elastic/eui#7373))

---------

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants