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

Dynamic starting page FE #13970

Merged
merged 52 commits into from Jan 26, 2023
Merged

Dynamic starting page FE #13970

merged 52 commits into from Jan 26, 2023

Conversation

maxiadlovskii
Copy link
Contributor

@maxiadlovskii maxiadlovskii commented Nov 15, 2022

This PR should be merged after BE. #13985

Description

In this PR we create a dynamic start page. On this page, users are able to see the most used content.

Motivation and Context

How Has This Been Tested?

To test this changes you have to wait until #13985 will be merged, or its also possible to run server from #13985 and then switch to brunch from this PR and take a look ant FE

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@maxiadlovskii maxiadlovskii changed the title New getting started page FE Dynamic start page FE Nov 22, 2022
Copy link
Contributor

@linuspahl linuspahl left a comment

Choose a reason for hiding this comment

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

Looks awesome! I added some suggestions.

<LinkContainer to={Routes.getting_started(true)}>
<MenuItem>Getting Started</MenuItem>
<LinkContainer to={Routes.welcome()}>
<MenuItem>Welcome</MenuItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the link, since we already redirect to the welcome page, when clicking on the logo.
In my opinion we can even remove the dropdown completely and redirect to the documentation when clicking on the icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have there Cluster Global API browser link. So I will remove just this one.

display: block;
`;

const EntityItem = ({ type, title, id }: LastOpenedItem) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we do not pass all LastOpenedItem attributes to this component lets create a separate Props type for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do. LastOpenedItem has just 3 props

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is, the Props type definitions looks like we always provide all attributes of ListOpenedItem as props for the component. But when we add another attribute for LastOpenedItem it would not be accessible as a component prop here, even though the the type definitions say so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Will fix it

graylog2-web-interface/src/components/welcome/helpers.ts Outdated Show resolved Hide resolved
if (!lastOpened.length) return <i>There are no last opened items</i>;

return (
<ListGroup data-testid="last-opened-list">
Copy link
Contributor

Choose a reason for hiding this comment

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

A nitpick, but I think it looks nice to remove the bottom margin of these lists. We could add className="no-bm".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have the same margins in other part of app

Copy link
Contributor

Choose a reason for hiding this comment

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

I am just saying it would look nice.

graylog2-web-interface/src/pages/StartPage.jsx Outdated Show resolved Hide resolved
graylog2-web-interface/src/routing/Routes.ts Outdated Show resolved Hide resolved
graylog2-web-interface/src/components/welcome/Welcome.jsx Outdated Show resolved Hide resolved
<StyledSectionComponent title="Last opened">
<LastOpenList />
</StyledSectionComponent>
<StyledSectionComponent title="Pinned items">
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about the name "Favourites"?

"Pinned" remindes me of the data table pinning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This naming is in the documentation. Also, BE already uses it. But ye... Favorites sound better. Let's talk with team about that

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, just a personal preference.

@maxiadlovskii maxiadlovskii changed the title Dynamic start page FE Dynamic starting page FE Nov 23, 2022
Copy link
Contributor

@linuspahl linuspahl left a comment

Choose a reason for hiding this comment

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

Look good, works well! Just added some suggestions for a final polishing.

&:hover {
color: ${theme.colors.variant.darker.primary};
}
`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a personal preference, but I can imagine that it is easier to understand, that this is a link, if we keep the default link styling.

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 made it in the same color as the relative label component.

Copy link
Contributor

Choose a reason for hiding this comment

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

We had the topic of differently coloured links in the past and usually got feedback that it would look better / be easier to understand if we color links the common way. I do not think the current version looks bad. Although the default color would probably look better with the dark mode version:

image

graylog2-web-interface/src/components/welcome/hooks.tsx Outdated Show resolved Hide resolved
search: { link: 'SEARCH_VIEWID', typeTitle: 'search' },
search_filter: { link: 'MY-FILTERS_DETAILS_FILTERID', typeTitle: 'search filter' },
unknown: { typeTitle: 'unknown', link: undefined },
};
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there are many entities the backend supports, which are not handled by the frontend yet.
The possible types are listed in GRNTypes.java. In other cases we use getShowRouteFromGRN to generate the link for an entity.

const entityType = entityTypeMap[type] ?? entityTypeMap.unknown;
const entityTypeTitle = useMemo(() => {
try {
return getTitleForEntityType(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add a parameter for getTitleForEntityType to define if the function should throw an error when and entity type is unknown. The parameter could be true by default.

@@ -0,0 +1,4 @@
type = "add"
message = "Dynamic starting page FE"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a more meaningful change log, like "Replacing getting started guide with start page which lists recently opened and favorite saved searches and dashboards and recent activity."

Last open - entities which were recently viewed.
Favorite items - entities which were added to the favorites.
Recent activity - shows all actions ( creating, updating, sharing e.t.c) which were made with all entities.
</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about displaying the description per section, below the section headline?

Regarding "last opened" and "favorite", in my opinion it makes sense to mention which entities are included in the lists. Currently it only contains saved searches and dashboards. It means we need to update the text when we extend the functionality, but I like that it gives users a clear understand of where / when the functionality is offered.

Last open
Overview of recently visited saved searches and dashboards.

Favorite items
Overview of your favorite saved searches and dashboards.

Recent activity
{if admin} This list includes all actions graylog users performed, like creating or sharing an entity.
{if non-admin user} Overview of actions you made with entities, like creating or sharing an entity.

Regarding "recent activity", it looks only an admin can see "creating", "updating" and "deleting" and anormal user just sees the activity for sharing, but I think we will change this soon.

Copy link
Contributor

@linuspahl linuspahl left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

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

🙏 🚀

@dennisoelkers dennisoelkers merged commit eb1bafe into master Jan 26, 2023
@dennisoelkers dennisoelkers deleted the feat/new-getting-started-page branch January 26, 2023 16:29
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.

None yet

3 participants