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

[webui] refactor overview page #2924

Merged
merged 14 commits into from
Oct 12, 2020
Merged

[webui] refactor overview page #2924

merged 14 commits into from
Oct 12, 2020

Conversation

Lijiaoa
Copy link
Contributor

@Lijiaoa Lijiaoa commented Sep 25, 2020

related issue: #2779 #2921

@Lijiaoa Lijiaoa marked this pull request as draft September 25, 2020 05:11
@QuanluZhang QuanluZhang mentioned this pull request Sep 25, 2020
79 tasks
@Lijiaoa Lijiaoa marked this pull request as ready for review September 28, 2020 06:07
@@ -30,7 +30,9 @@ export const AppContext = React.createContext({
// eslint-disable-next-line @typescript-eslint/no-empty-function, @typescript-eslint/no-unused-vars
changeMetricGraphMode: (val: 'max' | 'min') => {},
// eslint-disable-next-line @typescript-eslint/no-empty-function, @typescript-eslint/no-unused-vars
changeEntries: (val: string) => {}
changeEntries: (val: string) => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think (_: string) => {} will suppress the warning.

Why no-empty-function is enabled? Doesn't make any sense 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.

It makes sense to me. And this rule is discussed result?

return (
<React.Fragment>
<Stack className='config'>
<DefaultButton text='Config' onClick={showTrialConfigpPanel} />
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigpPanel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any details?

src/webui/src/static/function.ts Outdated Show resolved Hide resolved
@@ -12018,7 +12018,7 @@ selfsigned@^1.9.1:
resolved "https://registry.yarnpkg.com/selfsigned/-/selfsigned-1.10.7.tgz#da5819fd049d5574f28e88a9bcc6dbc6e6f3906b"
integrity sha512-8M3wBCzeWIJnQfl43IKwOmC4H/RAp50S8DF60znzjW5GVqTcSe2vWclt7hmYVPkKPlHWOu5EaWOMZ2Y6W8ZXTA==
dependencies:
node-forge "^0.10.0"
node-forge "0.9.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why such change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like the command yarn cause this change.

src/webui/src/static/function.ts Outdated Show resolved Hide resolved
<Stack horizontal className='panelTitle' style={{ marginTop: 10 }}>
<span style={{ marginRight: 12 }}>{tableListIcon}</span>
<span>Trial jobs</span>
<div style={{ backgroundColor: '#fff', marginTop: 18 }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest moving the style into trialsDetail.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. will to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add bulletedList className for background color style. and keep style={{ marginTop:18 }} because of priority(write this style in .bulletedList doesn't work)

@@ -116,6 +90,12 @@ class App extends React.Component<{}, AppState> {
this.setState({ bestTrialEntries: entries });
};

updateOverviewPage = (): void => {
this.setState(state => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the parenthesis is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually needed.

import '../../../static/style/overview/config.scss';

export const TrialConfigButton = (): any => {
const [isShowConfigPanel, setShowConfigPanle] = useState(false);
Copy link
Contributor

@liuzhe-lz liuzhe-lz Oct 9, 2020

Choose a reason for hiding this comment

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

Why using hook here?
I'm not familiar with this feature. But it looks less clear and "typed" to me at first glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hooks is useful for function component with [true, false] states. I upgrad react version is main to use hooks:)

@QuanluZhang QuanluZhang changed the base branch from master to v1.9 October 12, 2020 01:14
@QuanluZhang QuanluZhang merged commit 1cd7ad5 into microsoft:v1.9 Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants