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 and adaptive progress bar #47

Merged
merged 1 commit into from
Mar 30, 2020
Merged

Refactor and adaptive progress bar #47

merged 1 commit into from
Mar 30, 2020

Conversation

emettely
Copy link
Contributor

@emettely emettely commented Mar 26, 2020

Is your Pull Request request related to another issue in this repository ?
Fixes #40

bug-dpe

Describe what the PR does
See below in comments:

  • general refactoring
  • bug fix, adaptive progress bar that retains accuracy of where it can be clicked.

State whether the PR is ready for review or whether it needs extra work
Ready

Additional context
Post review tasks

  • Get it reviewed / merge to master
  • Release a new version by pushing the master changes to release
  • Ensure it works in the main DPE.

Copy link
Contributor Author

@emettely emettely left a comment

Choose a reason for hiding this comment

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

Very small and easy fix :)

Comment on lines +26 to +30
useEffect(() => {
if (offsetWidth !== width) {
setWidth(offsetWidth);
}
}, [ ref ]);
}, [ offsetWidth ]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to use Layout Effect. useEffect does not work very well with ref changes, as objects make it more difficult to detect changes.

Comment on lines +8 to +11
const secondsToHHMMSSFormat = (seconds) => {
return new Date(seconds * 1000).toISOString().substr(11, 8);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving it out of main component as it's a very generic function. Could be moved to a util file.

@@ -1,71 +1,66 @@
import React, { Component } from 'react';
import PropTypes from 'prop-types';
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

General refactor to move to a function based component.

Comment on lines +57 to +64
ProgrammeScriptEditor.propTypes = {
handleDelete: PropTypes.any,
handleEdit: PropTypes.any,
handleReorder: PropTypes.any,
items: PropTypes.any,
playlist: PropTypes.any,
title: PropTypes.any
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added propTypes.


export default ProgramScript;
export default ProgrammeScriptEditor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed, as it was being imported as ProgrammeScriptEditor

@@ -3,7 +3,6 @@ import React from 'react';
import { storiesOf } from '@storybook/react';
import { action } from '@storybook/addon-actions';
import ProgrammeScriptEditor from '../index.js';
import PreviewCanvas from '../../PreviewCanvas';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused import

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

after re-sizing the progress-bar, it loses accuracy
1 participant