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

ui: use StatementsPage from admin-ui-components #51910

Merged
merged 3 commits into from
Aug 5, 2020

Conversation

koorosh
Copy link
Collaborator

@koorosh koorosh commented Jul 27, 2020

Depends on cockroachdb/admin-ui-components#9
Depends on cockroachdb/yarn-vendored#27
Depends on cockroachdb/yarn-vendored#28

StatementsPage component is replaced with previously extracted version of StatementsPage in admin-ui-components.
The new version (extracted) of StatementsPage has several changes that required to make the following adjustments:

  • ActivateStatementDiagnostics modal isn't connected components anymore so dispatching actions are pulled up to connected StatementsPage wrapper.
  • Previously, analytics functions (which track user interaction) were
    called directly from React components that made this functionality tightly coupled.
    Now, sagas used to watch actions related to user interaction and then call async logic in sagas.
    -- components expose only callback functions which are related to the
    the logic component is responsible;
    -- connected component is responsible to dispatch actions with proper
    payload to be properly then handled by sagas.

@koorosh koorosh requested a review from a team July 27, 2020 11:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@support-dpulls
Copy link

Hey there, Walid from Dpulls here!
We noticed that you have created a PR that depends on another one on a different repository. However, Dpulls is currently not installed on the repository cockroachdb/admin-ui-components which will cause the dependency to not be resolved in case the PR cockroachdb/admin-ui-components#9 gets merged (because Dpulls won't receive the event).
So we suggest you to also install Dpulls on cockroachdb/admin-ui-components using this link https://github.com/apps/dpulls/installations/new.
There is an open issue to address this kind of problem dpulls/backlog#7. Basically in the future, if a dependency repo is not installed, the Dpulls bot will add a comment on the PR mentioning that and the status check won't be created.
Let me know if you need more information. Thanks!
cc @koorosh @jordanlewis

@dpulls
Copy link

dpulls bot commented Jul 30, 2020

⚠️ Dpulls not installed on repository cockroachdb/admin-ui-components. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Jul 30, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Jul 30, 2020

⚠️ Dpulls not installed on repository cockroachdb/admin-ui-components. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Jul 30, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Jul 30, 2020

⚠️ Dpulls not installed on repository cockroachdb/admin-ui-components. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Jul 30, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

@koorosh koorosh requested a review from dhartunian July 30, 2020 08:53
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

@koorosh the Statements Page storybook entry is currently broken. Can you take a look?

};
}

export function trackStatementsPaginationAction(pageNum: number): PayloadAction<number> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we also had tracking on column sort events, did I make that up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, David! indeed it has to be restored!

@koorosh
Copy link
Collaborator Author

koorosh commented Jul 31, 2020

@koorosh the Statements Page storybook entry is currently broken. Can you take a look?

@dhartunian the entire StatementsPage storybook is moved to admin-ui-component which consumes mocked data.
I suppose to remove this duplicate storybook here until we have a way to connect storybooks with the real server.
What do you think?

@dpulls
Copy link

dpulls bot commented Aug 3, 2020

⚠️ Dpulls not installed on repository cockroachdb/admin-ui-components. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Aug 3, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

@dhartunian
Copy link
Collaborator

@koorosh the Statements Page storybook entry is currently broken. Can you take a look?

@dhartunian the entire StatementsPage storybook is moved to admin-ui-component which consumes mocked data.
I suppose to remove this duplicate storybook here until we have a way to connect storybooks with the real server.
What do you think?

Yes we can remove. Sounds good 👍

Previously, analytics functions (which track user interaction) were
called directly from React components that made these functionality
tightly coupled.

Now, sagas used to watch actions related to user interaction and then
call async logic in sagas.
- components expose only callback functions which is related to the
logic component is responsible;
- connected component is responsible to dispatch actions with proper
payload to be properly then handled by sagas.

Release note: none
- Replace statementsPage component with extracted version
from `admin-ui-component` package.
- Provide correct props bindings to connected components,
because changed logic of calling analytics tracking functionality.

Release note: none
Initially, trackTableSort service was called directly from
React components and had an issue with correct tracking of
column names because `title` field has ReactNode type and
cannot be casted to String type for analytics payload.

With current change:
- trackTableSort service doesn't depend on interfaces defined on
representation level (decouples usage of service)
- trackTableSort service is called via sagas by watching appropriate
actions (again decouples service and representation layer)

Release note: None
@dpulls
Copy link

dpulls bot commented Aug 4, 2020

⚠️ Dpulls not installed on repository cockroachdb/admin-ui-components. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Aug 4, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Aug 4, 2020

⚠️ Dpulls not installed on repository cockroachdb/admin-ui-components. Checkout our quickstart for how to install.

@dpulls
Copy link

dpulls bot commented Aug 4, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

1 similar comment
@dpulls
Copy link

dpulls bot commented Aug 4, 2020

⚠️ Dpulls not installed on repository cockroachdb/yarn-vendored. Checkout our quickstart for how to install.

@koorosh koorosh requested a review from dhartunian August 4, 2020 14:59
@koorosh
Copy link
Collaborator Author

koorosh commented Aug 5, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 5, 2020

Build succeeded:

@craig craig bot merged commit 8f7a596 into cockroachdb:master Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants