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

Restyle developer tools UI and add a notification for new dash versions #3121

Draft
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

marthacryan
Copy link

@marthacryan marthacryan commented Jan 13, 2025

Updates developer tools to have a new UI.
dash-devtools

The notification prompts the developer to upgrade their version of dash and has 3 options for actions:

  • Don't show again -> sets a boolean flag in local storage if the user clicks.
  • Remind me later -> sets a field in local storage to the time that this button is clicked. The frontend then uses that to determine whether to show the notification again.
  • Skip this version -> sets a field in local storage to the version that was received from the server. The frontend then uses that to determine whether to show the notification again.
  • Read more -> links to dash documentation on installation

Note: Currently requires plotly VPN to test until we make the server public.

Todos:

  • Add style to UI (There is currently no style applied to the buttons / very little applied to the notification)
  • Update dev tools menu options - create separate button for toggling notifications?
  • Swap in actual server information for notifications
  • Code cleanup
    • Change names of files / directories to reflect actual content better
    • Do a pass through style to make sure there isn't extraneous css
  • Add tests
  • Some style cleanup?
    • Add different color for hover on tooltip buttons
    • Add font (inter?)
    • Make errors not scroll / take up more room
  • Documentation (@LiamConnors)
  • Finalize the server
    • Grab version from pypi
    • Make public
  • Implement it so that it's a feature flag that refreshes once a day or per refresh

@gvwilson gvwilson added feature something new P2 considered for next cycle labels Jan 15, 2025
@marthacryan marthacryan changed the title Add very basic update notifications Restyle developer tools UI and add a notification for new dash versions Jan 29, 2025
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Would be best to refactor the new version functionality in it's component file.
Also should add a flag in the config to disable the system entirely in an opt out value.


class DebugMenu extends Component {
constructor(props) {
super(props);
const {config} = props;

requestDashVersionInfo(config.dash_version).then(body => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right place for that call. I suggest creating a new functional component and using useEffect to trigger the call and attach the result to a redux action/reducer.

Copy link
Author

Choose a reason for hiding this comment

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

Done! Does that look ok? I use react state rather than redux.

};

// Close the upgrade tooltip if the user clicks outside of it
document.addEventListener('click', e => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This also need to be unattached if the component gets unmounted. Move to useEffect in the new component and have it return () => document.removeEventListener(...)

Copy link
Author

Choose a reason for hiding this comment

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

Done! Does that look ok?

Comment on lines 187 to 219
<div className='dash-debug-menu__version'>
{this.state.upgradeTooltipOpened ? (
<div className='dash-debug-menu__upgrade-tooltip'>
<a
target='_blank'
href='https://dash.plotly.com/installation'
>
Read details
</a>
<button onClick={setSkipThisVersion}>
Skip this version
</button>
<button onClick={setRemindMeLater}>
Remind me tomorrow
</button>
<button onClick={setDontShowAgain}>
Silence all version notifications
</button>
</div>
) : null}
<span>v{config.dash_version}</span>
{shouldShowUpgradeNotification(
config.dash_version,
newDashVersion
) ? (
<button
className='dash-debug-menu__upgrade-button'
onClick={toggleShowUpgradeTooltip}
>
Dash update available - v{newDashVersion}
</button>
) : null}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this part to a new component and maybe also a component for each entry that was previously buttonFactory but I think it could have been a component DebugButton

Copy link
Author

Choose a reason for hiding this comment

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

Good point! I moved the upgrade info (current version and if necessary the upgrade button / tooltip) into VersionInfo.react.js, let me know if that looks ok.

For the other buttons, they each have a bit of custom logic to the point that I'm not sure it will save any lines of code for them to be separate. But let me know if you think otherwise.

className={classes('dash-debug-menu__icon', 'debug')}
/>
</div>
<GlobalErrorOverlay
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comoponent not used anymore?

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah I did remove that. It had very little logic in it so I just moved it into the DebugMenu.react.js. The only thing I see it doing is concatenating the frontend and the backend errors, which I now do on line 148. Here's the current file:

const errors = concat(error.frontEnd, error.backEnd);

@marthacryan
Copy link
Author

Would be best to refactor the new version functionality in it's component file. Also should add a flag in the config to disable the system entirely in an opt out value.

We're still deciding how opt outs should happen so I'm going to wait on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants