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.

Sign up for GitHub

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

Jump to bottom

[Dashboard] [Controls] Refactor options list to align with new code standards #138336

Merged
merged 11 commits into from
Aug 12, 2022
Merged

[Dashboard] [Controls] Refactor options list to align with new code standards #138336

merged 11 commits into from
Aug 12, 2022

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Aug 8, 2022 •
edited
Loading

Summary

This PR serves as the first refactor of the options list control with the goal of aligning with the new code standards that the Presentation team is working on. It has absolutely no user-facing changes.

Checklist

For maintainers

@Heenawter Heenawter added Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. backport:skip This commit does not require backporting v8.5.0 ci:skip-when-possible (Deprecated) no-op, managed automatically labels Aug 8, 2022
@Heenawter Heenawter changed the title [Dashboard] [Controls] Refactor options list control to align with new embeddable standards [Dashboard] [Controls] Refactor options list to align with new embeddable standards Aug 9, 2022
@Heenawter Heenawter changed the title [Dashboard] [Controls] Refactor options list to align with new embeddable standards [Dashboard] [Controls] Refactor options list to align with new code standards Aug 9, 2022
@Heenawter Heenawter force-pushed the refactor-options-list_2022-08-08 branch from 738d0d3 to aaf7af1 Compare August 9, 2022 16:54
@Heenawter Heenawter force-pushed the refactor-options-list_2022-08-08 branch from 8c72b0b to 8cadc07 Compare August 9, 2022 19:37
@Heenawter Heenawter force-pushed the refactor-options-list_2022-08-08 branch from 8cadc07 to 0a37b38 Compare August 9, 2022 19:38
Heenawter
Heenawter commented Aug 10, 2022
View reviewed changes
src/plugins/controls/public/control_types/time_slider/time_slider_embeddable_factory.tsx
@@ -16,7 +16,7 @@ import { ControlEmbeddable, DataControlField, IEditableControlFactory } from '..
import {
createOptionsListExtract,
createOptionsListInject,
} from '../../../common/control_types/options_list/options_list_persistable_state';
} from '../../../common/options_list/options_list_persistable_state';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL why is time slider using the options list persistable state? Definitely something to fix in the time slider refactor @cqliu1 @ThomThomson 😆

@Heenawter Heenawter force-pushed the refactor-options-list_2022-08-08 branch 3 times, most recently from 9f75cd3 to 37fbfb4 Compare August 10, 2022 20:58
@Heenawter Heenawter force-pushed the refactor-options-list_2022-08-08 branch from 37fbfb4 to 0720d1b Compare August 10, 2022 20:59
@Heenawter
Copy link
Contributor Author

@elastic/kibana-design The only thing I did was move a .scss file to align with our new organization expectations - nothing actually changed from a user perspective :)

@Heenawter Heenawter marked this pull request as ready for review August 10, 2022 21:20
@Heenawter Heenawter requested review from a team as code owners August 10, 2022 21:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

andreadelrio
andreadelrio approved these changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Sass change LGTM.

@Heenawter Heenawter self-assigned this Aug 11, 2022
cqliu1
cqliu1 approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I pulled it down and options list still works as expected. One tiny nit but not a dealbreaker

src/plugins/controls/public/options_list/components/options_list_popover.tsx
}) => {
}

export const OptionsListPopover = ({ width, updateSearchString }: OptionsListPopoverProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This might be a personal preference thing, but I like using FC<Props>

Suggested change
export const OptionsListPopover = ({ width, updateSearchString }: OptionsListPopoverProps) => {
export const OptionsListPopover: FC<OptionsListPopoverProps> = ({ width, updateSearchString}) => {

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 believe most of the controls-related components follow the style without the FC<Props> so I think I'll stick with that? Honestly not a huge deal, but if we wanted to be that thorough about code consistency, it honestly would be easier to remove FC<Props> from everywhere because, at least in main right now, only time slider uses that syntax 😆

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 443.4KB 443.7KB +396.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
controls 29.3KB 28.6KB -749.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Heenawter

@Heenawter Heenawter merged commit e0085c4 into elastic:main Aug 12, 2022
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
@Heenawter @Mpdreamz
[Dashboard] [Controls] Refactor options list to align with new code s…
81ac63f 
…tandards (elastic#138336)

* Refactor public folder structure

* Refactor common folder structure

* Refactor server folder structure

* Stub out Jest tests and stories

* Add popover jest tests

* Fix strings + imports

* Remove comments and unused strings

* Fix jest tests

* Clean imports
@Heenawter Heenawter mentioned this pull request Sep 9, 2022
Merged
3 tasks
@Heenawter Heenawter mentioned this pull request Feb 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cqliu1 cqliu1 cqliu1 approved these changes

@andreadelrio andreadelrio andreadelrio approved these changes

Assignees

@Heenawter Heenawter

Labels
backport:skip This commit does not require backporting ci:skip-when-possible (Deprecated) no-op, managed automatically Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Heenawter @elasticmachine @kibana-ci @cqliu1 @andreadelrio