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

Refactor: Remove logoClickConfirm and make user go to the experiment dashboard directly #1303

Merged
merged 2 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 2 additions & 2 deletions frontend/src/components/AppBar/AppBar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ import Logo from "@/components/Logo/Logo";


// AppBar is a bar on top of the app, with navigation and title
const AppBar = ({ title, logoClickConfirm = null }) => {
const AppBar = ({ title, }) => {

return (
<div className="aha__app-bar navbar bg-black">
<Logo logoClickConfirm={logoClickConfirm} />
<Logo />
<h4 className="title text-light">{title}</h4>
<span className="action-right"></span>
</div>
Expand Down
11 changes: 0 additions & 11 deletions frontend/src/components/AppBar/AppBar.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,4 @@ describe('AppBar', () => {
expect(logo.getAttribute('href')).toBe(BASE_URL);
});

it('prevents navigation when logoClickConfirm is provided and user cancels', () => {
// Mock window.confirm
window.confirm = vi.fn(() => false);

const { getByLabelText } = render(<AppBar title="Test Title" logoClickConfirm="Confirm?" />, { wrapper: Router });
const logo = getByLabelText('Logo');
fireEvent.click(logo);

expect(window.confirm).toHaveBeenCalledWith('Confirm?');
});

});
8 changes: 0 additions & 8 deletions frontend/src/components/Experiment/Experiment.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -245,14 +245,6 @@ const Experiment = ({ match }) => {
{(!loadingExperiment && experiment) || view === "ERROR" ? (
<DefaultPage
title={state.title}
logoClickConfirm={
["FINAL", "ERROR"].includes(view) ||
// Info pages at end of experiment
(view === "INFO" &&
(!state.next_round || !state.next_round.length))
? null
: "Are you sure you want to stop this experiment?"
}
className={className}
>
{render(view)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const ExperimentCollectionDashboard: React.FC<ExperimentCollectionDashboa

return (
<div className="aha__dashboard">
<Logo logoClickConfirm={null} />
<Logo />
{headerProps && (
<Header {...headerProps}></Header>
)}
Expand Down
13 changes: 1 addition & 12 deletions frontend/src/components/Logo/Logo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,16 @@ import { Link } from "react-router-dom";
import useBoundStore from "@/util/stores";


const Logo: React.FC<{ logoClickConfirm: string | null }> = ({ logoClickConfirm = null }) => {
const Logo: React.FC = () => {
const theme = useBoundStore((state) => state.theme);

const { alt, title, file, target, rel } = theme?.logo || {};
const href = theme?.logo?.href || URLS.AMLHome;
const logoUrl = file ?? LOGO_URL;

// Handle click on logo, to optionally confirm navigating
const onLogoClick = (e) => {
if (logoClickConfirm) {
if (!window.confirm(logoClickConfirm)) {
e.preventDefault();
return false;
}
}
};

// Logo is a Link in case of relative url (/abc),
// and a-element for absolute urls (https://www.example.com/)
const logoProps = {
onClick: onLogoClick,
className: "aha__logo",
"aria-label": "Logo",
style: { backgroundImage: `url(${logoUrl})` },
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/components/Page/DefaultPage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import Page from "./Page";
import AppBar from "../AppBar/AppBar";

// DefaultPage is a Page with an AppBar and a width-restricted container for content
const DefaultPage = ({ className, title, logoClickConfirm, children }) => {
const DefaultPage = ({ className, title, children }) => {
return (
<Page className={className}>
<AppBar title={title} logoClickConfirm={logoClickConfirm} />
<AppBar title={title} />
<div className="container">
<div className="row justify-content-center py-3">
<div className="col-12">{children}</div>
Expand Down
1 change: 0 additions & 1 deletion frontend/src/components/Page/DefaultPage.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ describe("DefaultPage Component Tests", () => {
const defaultProps = {
className:'aha__default',
title: "Default page title",
logoClickConfirm: null,
collectionSlug: 'some_collection',
nextExperimentSlug: 'some_experiment',
}
Expand Down
Loading