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

Migrate Dashboard and Public Dashboard to React #4228

Merged
merged 50 commits into from
Dec 24, 2019
Merged

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Oct 8, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

Migration of the Dashboard to React

Refresh and Refresh rate
Same presets kept, now updates URL accordingly with refresh=[refreshRate].
No button on the Public Dashboard, but can be updated through the URL (as it was before).

refresh

Fullscreen
Now updates URL accordingly.
The button is still only visible for Published queries, lmk if you want that changed.

fullscreen

Publish/Unpublish
publish-unpublish

Mobile

  • Dashboard Header is no longer Sticky for Mobile (there was an issue with it now that the Navbar is fixed). Only con I see is in case you want to refresh the dashboard you'll have to go all the way up.
  • Refresh is now the only Button that appears in mobile (previously the "Publish" button would appear too)

Edit Mode

edit-mode

Public Dashboard
react-public-dashboard

Features

Bugs Found

  • Bug 🪲: Dashboard Level Filters not updating when it's changed.

Related Tickets & Documents

Closes #2076
Closes #4204
Closes #4341

@gabrieldutra gabrieldutra self-assigned this Oct 8, 2019
}

function useRefreshRateHandler(refreshDashboard, updateUrlSearch) {
const [refreshRate, setRefreshRate] = useState(getRefreshRateFromUrl());
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this now allows custom refresh rates 🕺

@kravets-levko
Copy link
Collaborator

@gabrieldutra prepareWidgetsForDashboard was needed for very old dashboards (in versions prior to 4 they were implemented as Bootstrap grid) - since there are no migration that updates dashboard grid between v3 and v4 - prepareWidgetsForDashboard was doing that job. Without it, users that will decide to upgrade from Redash v3 to latest (not sure if there are a lot of them) will get all their dashboards broken.

I know that it was an attempt to fix the issue with unnecessary API calls, but IMHO the solution should be different: there is a dirty-check mechanism for auto-save layout feature - this mechanism should be disabled in non-edit mode (as it was prior to Dashboard grid migration to React).

@arikfr
Copy link
Member

arikfr commented Nov 14, 2019

I agree with @kravets-levko -- we should keep the migration mechanism and implement the solution only for saves during non edits.

@gabrieldutra
Copy link
Member Author

Thanks @kravets-levko, I didn't know about that. Moved to option B to be closer to how this was done prior to the autosave: disabled layout saving when not in edit mode.

@susodapop
Copy link
Contributor

This passes the sanity check from my end-user perspective. I like the new button look.

@gabrieldutra
Copy link
Member Author

Thanks, @susodapop 🙂


function RefreshButton({ dashboardOptions }) {
const { refreshRate, setRefreshRate, refreshing, refreshDashboard } = dashboardOptions;
const refreshRateOptions = clientConfig.dashboardRefreshIntervals;
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be missing the use of a policy to define allowed refresh intervals:

const allowedIntervals = policy.getDashboardRefreshIntervals();
if (_.isArray(allowedIntervals)) {
_.each(this.refreshRates, (rate) => {
rate.enabled = allowedIntervals.indexOf(rate.rate) >= 0;
});
}

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔, currently it's still possible to put any interval you want through the url, should it be reflected in there?

Copy link
Member

Choose a reason for hiding this comment

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

I think it was possible before as well, but we can also make sure that the value in the url is not lower than the minimum value the policy allows. Something like:

const refreshRate = max(urlValue, lowestPolicyValue);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my currently was meant to be master version, but cool, will go with it 👍


useEffect(() => {
document.title = dashboard.name;
}, [dashboard.name]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@arikfr arikfr merged commit 38b6b47 into master Dec 24, 2019
@arikfr arikfr deleted the react-dashboard branch December 24, 2019 08:20
@arikfr
Copy link
Member

arikfr commented Dec 24, 2019

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants