Refactor header using GravityUI #566

Kabir-Ivan · 2024-07-05T14:20:34Z

KuznetsovRoman

I would also like to see the screenshots (of that browser list especially)

KuznetsovRoman · 2024-07-09T11:26:29Z

test/func/tests/screens/3144090/chrome/menu bar plugins clicked.png

Why did the menu went in the left?
It should stay at the right

KuznetsovRoman · 2024-07-09T11:27:05Z

test/func/tests/common/test-results-appearance.testplane.js

-        await browser.$('button*=Expand all').click();
+        await browser.$('//*[contains(@class, "expand-dropdown")]//button').click();
+        await browser.$('//*[contains(@class, "g-popup")]//span[contains(normalize-space(), "Expand all")]').click();
+        // await browser.$('//label[contains(normalize-space(), "Expand all")]').click();


Commented code should be removed

KuznetsovRoman · 2024-07-09T11:31:31Z

lib/static/components/controls/browser-list/index.jsx

            }
+            versions.forEach((version) => {
+                hasSpecialGroup = true;


You don't need to assign true to hasSpecialGroup in every iteration of this forEach loop

KuznetsovRoman · 2024-07-09T11:35:07Z

lib/static/components/controls/browser-list/index.jsx

+    const getOptions = () => {
+        const groups = {};
+        const DEFAULT_GROUP = "other";
+        let hasSpecialGroup = false;


From its name, it is unobvious, what makes the group "special"
Variable name should be more self-explanatory. For example: "hasNestedOptions", or "hasMultipleVersions", etc...

KuznetsovRoman · 2024-07-09T11:38:51Z

lib/static/components/controls/browser-list/index.jsx

-
-            if (!isEmpty(versions) && versions.length > 1) {
-                node.children = versions.map(version => mkNode({browserId, version}));
+    const rawOptions =  useMemo(getOptions, [available]);


Double "space" between "=" and "useMemo" here and at line#96
Interesting, how didn't eslint notice that.

KuznetsovRoman · 2024-07-09T12:41:22Z

lib/static/components/controls/expand-input.jsx

Let's write new files in typescript: ".tsx"

KuznetsovRoman · 2024-07-09T12:44:32Z

lib/static/components/controls/run-button/index.jsx

@@ -46,6 +47,18 @@ const RunButton = ({actions, autoRun, isDisabled, isRunning, failedTests, checke
        action();
    };

+    const handleSelect = (values) => {
+        if (values.length) {


How could there be multiple values?

KuznetsovRoman · 2024-07-09T12:46:39Z

lib/static/components/header/summary/dbSummary.jsx

+        // const additionalInfo = fetchDbDetails.map(({url, status, success}) => (
+        //     <Menu.Item key={url}>
+        //         {' '}
+        //         {url} responded with
+        //         <span className={success ? 'db-info__row_success' : 'db-info__row_fail'}>{' ' + status}</span>
+        //     </Menu.Item>
+        // ));


Commented code should be removed

KuznetsovRoman · 2024-07-09T12:47:09Z

lib/static/components/header/summary/dbSummary.jsx

+            <Popover
+            disablePortal
+            placement={'bottom'}
+                content={
+                    additionalInfo
+                    }
            >


Strange formatting.
Have you launched npm run lint -- --fix?

KuznetsovRoman · 2024-07-09T12:47:40Z

lib/static/components/header/summary/summary.css

@@ -34,19 +35,15 @@
    display: inline;
 }

-.db-info__popup {
+.db-info-container {
+    --g-popover-max-width: 5000px;


Why 5000px?

KuznetsovRoman · 2024-07-10T13:16:48Z

lib/static/components/controls/browser-list/index.styl

+.browserlist__row:hover
+    .action-button
+        opacity 1


It should be:

.g-select-list__option:hover .browserlist__row .action-button opacity 1

That way the "Only"/"Except" button will be shown while hovering the entire line, including the check mark.
Currently "Only"/"Except" is not shown while hovering the check mark

KuznetsovRoman

/ok

fix: fix copy button feat: use gravity ui in header refactor: refactor header using gravityUI test: fix tests refactor: use gravity ui in menu bar test: debug test: add retries refactor: refactor code

shadowusr · 2024-07-12T10:09:56Z

lib/static/components/controls/browser-list/index.jsx

+        if (!hasNestedOptions) {
+            return groups[DEFAULT_GROUP];
+        }
+        else {


A few remarks about code style:

There are almost no blank lines at all. It makes code harder to read. Look at how the code was structured before — we usually divide code into sections (grouped by meaning) with blank lines between them. For instance, it's almost always a good idea to insert a blank line before return statement (unless it's the only statement in the code block e.g. if (...) { return ...; }

We usually write else clauses like this } else {. But here we don't need else clause altogether — it just introduces unnecessary level of nesting.

shadowusr · 2024-07-12T10:12:36Z

lib/static/components/controls/browser-list/index.jsx

+                    label: name,
+                    options: groups[name]
+                })
+            })


Here, there's no semicolon. And this is not the only place. Don't we have a linter rule turned on for that? Can you please check? eslint should issue warnings/errors for missing semicolons.

shadowusr · 2024-07-12T10:27:16Z

lib/static/components/controls/browser-list/index.jsx

+        });
+        return mapping;
+    }
+    const getSelected = () => {


This is not a descriptive&clear name either. By reading getSelected I don't understand at all what this function will return — will it return a single selected string? Or a selected element? Or will it return an array of them?

I suggest something like getSelectedBrowserIds. Feel free to choose the other name, but it should be clear what function does just by looking at its name.

shadowusr · 2024-07-12T10:32:32Z

lib/static/components/controls/browser-list/index.jsx

-                });
+import { Button, Select, useSelectOptions } from '@gravity-ui/uikit';
+
+const BrowserList = ({available, onChange, selected: selectedProp}) => {


I see that selectedProp was here before, but I think it's bad naming too — it doesn't reflect what the variable holds at all. Let's name it something like selectedBrowsersProp, or you can try to come up with a better name.

and maybe available -> availableBrowsersProp too. This would indicate that these props store values of similar shape.

shadowusr · 2024-07-12T11:14:17Z

lib/static/components/controls/browser-list/index.jsx

-    if (isEmpty(available)) {
-        return (<div></div>);
+        return (
+        <div className='browserlist__row'>


Right now when you click on only/except the width of the popup changes, but it shouldn't.

Kabir-Ivan force-pushed the use-gravity-ui branch 2 times, most recently from 620327d to 80bb4e6 Compare July 8, 2024 07:50

KuznetsovRoman requested changes Jul 9, 2024

View reviewed changes

Kabir-Ivan force-pushed the use-gravity-ui branch from ffea142 to 4bd408b Compare July 10, 2024 11:30

KuznetsovRoman reviewed Jul 10, 2024

View reviewed changes

KuznetsovRoman approved these changes Jul 12, 2024

View reviewed changes

Kabir-Ivan added 3 commits July 12, 2024 14:11

refactor: refactor header using GravityUI

c398a0e

fix: fix copy button feat: use gravity ui in header refactor: refactor header using gravityUI test: fix tests refactor: use gravity ui in menu bar test: debug test: add retries refactor: refactor code

refactor: minor changes

b037f25

refactor: fix browser select

abad3fc

Kabir-Ivan force-pushed the use-gravity-ui branch from 4684fe6 to abad3fc Compare July 12, 2024 11:11

Kabir-Ivan merged commit e68d6be into master Jul 12, 2024
4 checks passed

shadowusr reviewed Jul 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor header using GravityUI #566

Refactor header using GravityUI #566

Kabir-Ivan commented Jul 5, 2024 •

edited

Loading

KuznetsovRoman left a comment

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 9, 2024

KuznetsovRoman Jul 10, 2024

KuznetsovRoman left a comment

shadowusr Jul 12, 2024

shadowusr Jul 12, 2024

shadowusr Jul 12, 2024

shadowusr Jul 12, 2024

shadowusr Jul 12, 2024

Refactor header using GravityUI #566

Refactor header using GravityUI #566

Conversation

Kabir-Ivan commented Jul 5, 2024 • edited Loading

KuznetsovRoman 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

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

KuznetsovRoman 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

Kabir-Ivan commented Jul 5, 2024 •

edited

Loading