Update UI dependencies #33

deadlycoconuts · 2022-09-02T10:21:43Z

Context

With the update of mlp-ui dependencies in caraml-dev/mlp#54 and caraml-dev/mlp#55 (in particular the update of Node.js 14 → 16, React 16 → 17, @elastic/eui 32.3.0 → 66.0.0 and react-scripts to 5.0.1), there is a need to similarly update XP UI, as it uses components from and is dependent on the mlp-ui package.

Main Modifications

Replacement of the existing Empty component with the new Page404 component
Extensive adoption of the new EuiPageTemplate (see https://elastic.github.io/eui/#/templates/page-template) to manage page layouts; this replaces the existing usage of EuiPage, EuiPageHeader, EuiPageSection, etc. components
Update of the existing colour palette manually specified to align with those of v66 of @elastic/eui (see https://eui.elastic.co/#/theming/colors/values)
Removal of redundant CSS styling used to accommodate EuiFlyouts
Replacement of the caret toggle used inn Segmenter cards with a gear

Fixes

Inconsistent spacing between page title and MLP bar

Before

Experiment/Treatment/Segment Details View: Experiment/Treatment/Segment Edit View:
.

After

Experiment/Treatment/Segment Details View: Experiment/Treatment/Segment Edit View:
.

Known Issues (Unable to be addressed in this PR)

To follow up on mlp-ui to fix the StepsWizardHorizontal component as it does not seem to be highlighting as users progress through the timeline steps:

ui/src/experiments/list/search/SearchExperimentsPanel.js

krithika369 · 2022-09-08T09:19:47Z

This PR is larger than the one for Turing. 😱 😅

@terryyylim could you have the first pass at this when you get to it? I'll be the second set of eyes this time.

terryyylim

Largely LGTM, just some small comments to address. Thank you for the PR, @deadlycoconuts!

terryyylim · 2022-09-09T02:48:17Z

ui/src/experiments/details/ExperimentDetailsView.js

+        </EuiCallOut>
+      ) : (
+        <Fragment>
+          {!(props["*"] === "edit") ? (


Could we do {!(props["*"] === "edit") && (...) here instead so we can be more concise and remove <></>?

terryyylim · 2022-09-09T02:53:43Z

ui/src/experiments/history/ListExperimentHistoryView.js

-      </EuiPanel>
-    </ConfigSection>
+    <Fragment>
+      <EuiSpacer size="m" />


What happens if this EuiSpacer gets removed?

This happens

vs

terryyylim · 2022-09-09T02:55:23Z

ui/src/experiments/history/details/ExperimentHistoryDetailsView.js


 const ExperimentHistoryDetailsView = ({ projectId, experimentId, version }) => {
+  const { appConfig } = useConfig();


Should we standardise with other files, to just extract variables we need? i.e restrictWidth and paddingSize in this case.

terryyylim · 2022-09-09T03:02:03Z

ui/src/experiments/list/search/SearchExperimentsFilters.js

@@ -78,14 +76,14 @@ const SearchExperimentFilters = ({ onChange }) => {

  return (
    <>
-      <EuiFlyoutHeader hasBorder className="searchPanelFlyoutHeader">


It just works? 🎉 Just curious, was this somehow fixed in some version of EUI which was after our current version?

Yea it just does. I don't know why the original header's margin was stretched by an additional 20px to the right; maybe it's to enable the 'Reset' button to lie further right than could be done normally?

The same effect is now achieved by making the EuiFlexItem containing the button not grow, which causes the EuiFlexGroup to allocate more space for the 'Filters' header instead:

ui/src/experiments/list/search/SearchExperimentsPanel.js

ui/src/segments/create/CreateSegmentView.js

ui/src/segments/details/SegmentDetailsView.js

ui/src/settings/details/SettingsDetailsView.js

ui/src/settings/edit/EditSettingsView.js

ui/src/settings/segmenters/details/SegmenterDetailsView.js

krithika369

I've partially reviewed this PR and left a few small comments. Will complete it on Monday.

ui/src/components/config_section/ConfigPanel.scss

krithika369 · 2022-09-09T07:46:52Z

ui/src/components/table/BasicTable.js

@@ -34,7 +34,7 @@ export const BasicTable = ({
      iconType="alert">
      <p>{error.message}</p>
    </EuiCallOut>
-  ) : !!onPaginationChange && !!totalItemCount & !!page ? (
+  ) : !!onPaginationChange && !!totalItemCount && !!page ? (


Thanks for catching this. 😅

ui/src/experiments/list/search/SearchExperimentsPanel.js

ui/src/settings/components/playground_flyout/PlaygroundFlyout.scss

ui/src/experiments/list/search/SearchExperimentsPanel.scss

terryyylim

LGTM! Thanks for the changes @deadlycoconuts 👍🏻 We can merge it once @krithika369 has another look.

krithika369

LGTM too. Thanks for this PR and refactoring some of the components along the way. I left some small comments. Please feel free to merge once they have been addressed.

ui/src/experiments/list/search/SearchExperimentsPanel.scss

krithika369 · 2022-09-13T00:37:07Z

ui/src/services/segmenter/SegmenterScope.js

@@ -2,11 +2,11 @@ export const getSegmenterScope = (scope) => {
  const status = {
    project: {
      label: "Project",
-      color: "primary",
+      color: "#07C",


Do primary / success no longer work for this?

Ah I made a mistake with the changes here; I thought the primary colours weren't working with primary/success specified because they seemed to be of a paler shade (so I changed them manually to the brighter ones we see in other parts of the new EUI theme):

But after checking out the docs again for EuiBadge, it seems like they are indeed of the intended shade:

I'll change these values back to primary/success in this case then 😅

Yeah the default color palette is fine. Not sure why the Eui team decided to keep the old colors for some components like Text and this one..

krithika369 · 2022-09-13T00:40:51Z

ui/src/services/experiment/ExperimentStatus.js

@@ -19,19 +19,19 @@ export const getExperimentStatus = (experiment) => {
    },
    Completed: {
      label: "Completed",
-      color: "#017D73",
+      color: "#00BFB3",


Can we replace the color values here with the healthColor values (and replace the property name healthColor with color in the app)? The current color values are only used in the Eui badge from what I see and they should be able to accept primary / success / ...

Similar issue here as the above - it seems like EuiHealth also uses a darker shade (kind of like in the older versions of the theme) of all the colours instead of the more exciting ones we've been seeing. I should checked this more thoroughly before I guess 👀 In any case, I've replaced the color values with the healthColor values (subdued/success/warning/primary) and removed the healthColor values completely. Any component that used to consume healthColor values now consume the new (reverted) color values.

ui/src/services/segmenter/SegmenterStatus.js

krithika369 · 2022-09-13T00:44:41Z

ui/src/settings/components/form/components/segmenter_section/SegmenterSettings.js

+      initialIsOpen={variables.length > 1}
+      buttonContent={buttonContent}
+      arrowDisplay="none"
+      extraAction={


Nice! Thanks for this.

ui/src/settings/components/playground_flyout/PlaygroundFlyout.scss

…alls

…ents and segments

deadlycoconuts · 2022-09-13T03:36:44Z

Thank you @terryyylim and @krithika369 for the detailed review! I should've addressed all (if not most 👀) of all the comments that both of you have left 🙂 I'll be merging this soon once the CI pipeline passes 🚀

deadlycoconuts self-assigned this Sep 2, 2022

deadlycoconuts commented Sep 5, 2022

View reviewed changes

ui/src/experiments/list/search/SearchExperimentsPanel.js Outdated Show resolved Hide resolved

deadlycoconuts marked this pull request as ready for review September 7, 2022 06:27

deadlycoconuts requested a review from a team September 7, 2022 06:27

terryyylim suggested changes Sep 9, 2022

View reviewed changes

krithika369 reviewed Sep 9, 2022

View reviewed changes

deadlycoconuts commented Sep 12, 2022

View reviewed changes

ui/src/settings/components/playground_flyout/PlaygroundFlyout.scss Outdated Show resolved Hide resolved

deadlycoconuts commented Sep 12, 2022

View reviewed changes

ui/src/experiments/list/search/SearchExperimentsPanel.scss Outdated Show resolved Hide resolved

deadlycoconuts requested a review from terryyylim September 12, 2022 02:41

terryyylim approved these changes Sep 12, 2022

View reviewed changes

krithika369 approved these changes Sep 13, 2022

View reviewed changes

deadlycoconuts added 18 commits September 13, 2022 09:55

Update ui dependencies and common files

5eed6f8

Update rest of the pages in xp to use EuiPageTemplate

be7838f

Fix formatting in landing view imports

77f89b2

Fix package.json script

b6ddbe3

Fix flyout styling

2539c3c

Remove redundant styling to accommodate flyouts

1296ca4

Replace EuiPage with EuiPageTemplate for experiment history view

18c1b94

Refactor UI pages to achieve consistent spacing

b165f31

Restore React imports

0f65075

Update eui version

3104749

Replace arrow caret in segmenter card with gear icon

e3340e8

Make experiment filters and playground flyout push out into the body

c7c5c42

Hide per page option from all tables displaying results of list API c…

aec2f86

…alls

Remove deprecated popperProps prop of EuiDatePicker

9610867

Make reset button in experiments filter not grow automatically

5c5ffd7

Remove autoFocus prop being set as true from EuiComponent

1cd42c7

Hide per page option from list version tables for experiments, treatm…

2a4839c

…ents and segments

Refactor conditional component rendering

4d0fc84

deadlycoconuts added 3 commits September 13, 2022 09:55

Standardise extraction of config variables

69f8070

Replace relative import paths with absolute paths

637e2b2

Move flyout width styling to scss files

91d4c9b

deadlycoconuts force-pushed the update_ui_dependencies branch from 6575502 to 91d4c9b Compare September 13, 2022 02:01

deadlycoconuts added 3 commits September 13, 2022 10:50

Revert changes to segmenter scope badge colours

f38ffb3

Remove healthColor values from experiment and segmenter status

102585e

Refactor flyout custom styles separately as a special class

0a6fa97

deadlycoconuts merged commit 6bd3be9 into caraml-dev:main Sep 13, 2022

deadlycoconuts deleted the update_ui_dependencies branch September 13, 2022 08:03

krithika369 mentioned this pull request Sep 16, 2022

Add additional experiment metadata to Fetch Treatment response #36

Merged

terryyylim mentioned this pull request Sep 20, 2022

Fix UI regressions caraml-dev/mlp#59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update UI dependencies #33

Update UI dependencies #33

deadlycoconuts commented Sep 2, 2022 •

edited

Loading

krithika369 commented Sep 8, 2022

terryyylim left a comment

terryyylim Sep 9, 2022

terryyylim Sep 9, 2022

deadlycoconuts Sep 11, 2022

terryyylim Sep 9, 2022

terryyylim Sep 9, 2022

deadlycoconuts Sep 11, 2022

krithika369 left a comment

krithika369 Sep 9, 2022

terryyylim left a comment

krithika369 left a comment

krithika369 Sep 13, 2022

deadlycoconuts Sep 13, 2022

krithika369 Sep 13, 2022

krithika369 Sep 13, 2022

deadlycoconuts Sep 13, 2022 •

edited

Loading

krithika369 Sep 13, 2022

deadlycoconuts commented Sep 13, 2022


		const ExperimentHistoryDetailsView = ({ projectId, experimentId, version }) => {
		const { appConfig } = useConfig();

Update UI dependencies #33

Update UI dependencies #33

Conversation

deadlycoconuts commented Sep 2, 2022 • edited Loading

Context

Main Modifications

Fixes

Inconsistent spacing between page title and MLP bar

Before

After

Known Issues (Unable to be addressed in this PR)

krithika369 commented Sep 8, 2022

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryyylim left a comment

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts commented Sep 13, 2022

deadlycoconuts commented Sep 2, 2022 •

edited

Loading

deadlycoconuts Sep 13, 2022 •

edited

Loading