Skip to content

feat -- configurable homepage widgets #2047

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

Merged
merged 26 commits into from
Dec 21, 2022
Merged

feat -- configurable homepage widgets #2047

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ebbf018
Hard code test user name temporarily
B-T-D Nov 29, 2022
50e2e5a
Group SearchBar with other home page widget components, and move home…
B-T-D Dec 1, 2022
366f472
remove hardcoded username
B-T-D Dec 1, 2022
23cea12
run lint:fix
B-T-D Dec 1, 2022
cb0904f
Updates required by eslint
B-T-D Dec 2, 2022
c1e4358
More linter fixes
B-T-D Dec 5, 2022
4ad1ff0
tab index numbering and period char
B-T-D Dec 5, 2022
cabcac9
typo
B-T-D Dec 5, 2022
d7a7bda
nit in stylesheet; go back to previous syntax for retrieving callback…
B-T-D Dec 5, 2022
b649311
Use destructuring for callbacks in props
B-T-D Dec 6, 2022
45d88d6
# This is a combination of 2 commits.
B-T-D Dec 6, 2022
b80323d
WIP on getHomePageWidgets
B-T-D Dec 7, 2022
f4e8fa9
Merge branch 'btd-getHomepageWidgets'
B-T-D Dec 7, 2022
9f413d1
WIP squashable
B-T-D Dec 7, 2022
2a6010f
Static refactor to use non-dynamic getHomePageWidgets
B-T-D Dec 7, 2022
ab973a9
Rename components to 'Widget' and change Widget to an interface
B-T-D Dec 8, 2022
3479e64
Get default widgets via the function
B-T-D Dec 8, 2022
32d399e
Widgets interface
B-T-D Dec 9, 2022
15f1f9b
Cleanups
B-T-D Dec 9, 2022
765351d
comment cleanup
B-T-D Dec 9, 2022
be284c8
Update documentation
B-T-D Dec 16, 2022
8368e14
Btd wip homepage widgets (#3)
B-T-D Dec 16, 2022
85f5e77
Remove unneccessary divs
B-T-D Dec 16, 2022
d56f394
Cleanups
B-T-D Dec 16, 2022
189f44c
Use array.map() in getHomePageWidgetComponents
B-T-D Dec 16, 2022
16dc58c
Add test for new config-utils method
B-T-D Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion frontend/amundsen_application/static/css/_popovers.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@
width: 24px;
}


// Modals
.modal-header {
text-align: left;
Expand Down
67 changes: 45 additions & 22 deletions frontend/amundsen_application/static/js/pages/HomePage/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,57 @@ export interface DispatchFromProps {
searchReset: () => UpdateSearchStateReset;
}

export type Widget = {
// TODO: What file does the Widget type belong in?
name: string;
options: {
path: '/';
};
};

export type HomePageLayout = Widget[];

export type HomePageProps = DispatchFromProps & RouteComponentProps<any>;

export class HomePage extends React.Component<HomePageProps> {
componentDidMount() {
this.props.searchReset();
}

getHomePageWidgets(layout: HomePageLayout): React.ReactNode[] {
/* TODO: Make 100% sure it returns the pre-existing layout absent a custom config,
to not break any OSS users' Amundsen implementations */

return [
<div className="home-element-container">
<SearchBar />
</div>,
<div className="filter-breadcrumb pull-right">
<Breadcrumb
direction="right"
path="/search"
text={SEARCH_BREADCRUMB_TEXT}
/>
</div>,
Copy link
Member

Choose a reason for hiding this comment

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

We probably want this bundled inside the SearchWidget

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 agree conceptually but would like to leave that change until a little later if you don't mind--it'll require some tweaking beyond just moving the Breadcrumb to be inside a different render, since elsewhere in the UI we render the search bar without the "Advanced Search" Breadcrumb, for example on the advanced search page itself ('/search' URL):

Screenshot 2022-12-08 at 7 27 18 PM

(screenshot from our prod)

<div className="home-element-container">
<BadgesListContainer shortBadgesList />
</div>,
<div className="home-element-container">
<TagsListContainer shortTagsList />
</div>,
<div className="home-element-container">
<MyBookmarks />
</div>,
<div className="home-element-container">
<PopularTables />
</div>,
];
}

defaultLayout: HomePageLayout = [];

homePageWidgets = this.getHomePageWidgets(this.defaultLayout);

render() {
/* TODO, just display either popular or curated tags,
do we want the title to change based on which
Expand All @@ -47,28 +91,7 @@ export class HomePage extends React.Component<HomePageProps> {
}`}
>
<h1 className="sr-only">{HOMEPAGE_TITLE}</h1>
<div className="home-element-container">
<SearchBar />
</div>
<div className="filter-breadcrumb pull-right">
<Breadcrumb
direction="right"
path="/search"
text={SEARCH_BREADCRUMB_TEXT}
/>
</div>
<div className="home-element-container">
<BadgesListContainer shortBadgesList />
</div>
<div className="home-element-container">
<TagsListContainer shortTagsList />
</div>
<div className="home-element-container">
<MyBookmarks />
</div>
<div className="home-element-container">
<PopularTables />
</div>
{this.homePageWidgets}
Copy link
Member

Choose a reason for hiding this comment

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

There are several approaches to this, you can see 'generateHomepageWidgets()' functions around, and also having new components rendered:
I favor the last one, but the former is common in Amundsen's codebase too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by having new components rendered? A new HomePageWidgetContainer (or whatever name) component that holds all the widgets that the user wants on their homepage? That seems logical to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it could be something like:

  render() {
    /* TODO, just display either popular or curated tags,
    do we want the title to change based on which
    implementation is being used? probably not */
    return (
      <main className="container home-page">
        <div className="row">
          <div
            className={`col-xs-12 ${
              announcementsEnabled() ? 'col-md-8' : 'col-md-offset-1 col-md-10'
            }`}
          >
            <h1 className="sr-only">{HOMEPAGE_TITLE}</h1>
            ****<HomePageWidgets
               getHomePageWidgets={getHomePageWidgets} 
             />****
          </div>
          {announcementsEnabled() && (
            <div className="col-xs-12 col-md-offset-1 col-md-3">
              <Announcements />
            </div>
          )}
        </div>
      </main>
    );
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but you don't need to extract it into a different file for the moment, you can write it above line 40 as a functional component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will do!

</div>
{announcementsEnabled() && (
<div className="col-xs-12 col-md-offset-1 col-md-3">
Expand Down