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

feat: Add cloud env setting page and notification server page #682

Merged
merged 11 commits into from
Jan 18, 2019

Conversation

liiil825
Copy link
Contributor

@liiil825 liiil825 commented Jan 16, 2019

solve #596

@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #682 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #682   +/-   ##
======================================
  Coverage    26.2%   26.2%           
======================================
  Files         102     102           
  Lines        1557    1557           
  Branches      355     355           
======================================
  Hits          408     408           
  Misses       1121    1121           
  Partials       28      28
Impacted Files Coverage Δ
src/components/Base/Checkbox/checkbox.jsx 80% <ø> (ø) ⬆️
src/components/Layout/SideNav/index.jsx 2.81% <ø> (ø) ⬆️
src/components/Layout/SideNav/navMap.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20ea9bf...61466fe. Read the comment docs.

@liiil825 liiil825 requested review from sunnywx and whDongRui January 16, 2019 08:48
@@ -47,11 +47,10 @@ export const getNavs = {
title: 'Users'
},
{
link: '#',
link: '/dashboard/cloud-environment',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not set link as /dashboard/settings ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

暂时只完成了云环境页面,先让这里跳转到

/dashboard/settings 表示云环境页面不太合适

Copy link
Contributor

Choose a reason for hiding this comment

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

可以类似 /dashboard/settings/:term, term 代表不同的子项,云环境为 /:dash/settings/cloud-env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我试过,现在这样弄会导致一级导航选不中,需要改造 SideNav 组件

},
{
name: 'Cloud environment',
link: '/dashboard/cloud-environment',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe link is complex

export default class CloudEnvironment extends Component {
async componentDidMount() {
const { cloudEnvironmentStore } = this.props;
cloudEnvironmentStore.fetchAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

missing await

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

<div
key={item.id}
className={classnames(styles.item, {
[styles.disabled]: !checked
Copy link
Contributor

Choose a reason for hiding this comment

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

too verbose. Why not

[styles.disabled]: item.status !== 'active'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

下面也用了 checked 判断,复用下

Copy link
Contributor

Choose a reason for hiding this comment

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

好吧...

cloudEnvironmentStore.fetchAll();
}

renderItem = item => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can destruct item

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

@@ -71,6 +71,8 @@ const routes = {

'/:dash/account/:type?': Dash.Account,

'/:dash/cloud-environment': [Dash.CloudEnvironment, { acl: r.admin }],
Copy link
Contributor

Choose a reason for hiding this comment

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

give a short and simple path

@@ -184,5 +186,7 @@ export default class RootStore extends Store {

// Vendor
this.register('vendor', Vendor);

this.register('cloudEnvironment', CloudEnvironment);
Copy link
Contributor

Choose a reason for hiding this comment

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

cloud env is the same as runtime provider

fetchAll = async () => {
const result = mockData;
this.environment = _.get(result, 'environment_set', []);
const totalCount = _.get(result, 'total_count', 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

totalCount is also observable

import { observable, action } from 'mobx';
import _ from 'lodash';

import mockData from './mock-data';
Copy link
Contributor

Choose a reason for hiding this comment

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

remove mock data

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

@@ -0,0 +1,39 @@
export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need create this file, you can use config/runtimes.js

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

@sunnywx sunnywx changed the title feat: Add cloud environment page feat: Add cloud env setting page Jan 16, 2019
@sunnywx sunnywx changed the title feat: Add cloud env setting page [WIP] feat: Add cloud env setting page Jan 16, 2019
@liiil825 liiil825 force-pushed the feat/cloud-environment branch from bc1c3d9 to 150a3b3 Compare January 18, 2019 03:53
@openpitrix openpitrix deleted a comment Jan 18, 2019
@openpitrix openpitrix deleted a comment Jan 18, 2019
@openpitrix openpitrix deleted a comment Jan 18, 2019
@openpitrix openpitrix deleted a comment Jan 18, 2019
@liiil825 liiil825 changed the title [WIP] feat: Add cloud env setting page feat: Add cloud env setting page and notification server page Jan 18, 2019
'dashboard'
];
const changeKey = {
'cloud-env': 'setting',
Copy link
Contributor

Choose a reason for hiding this comment

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

我看下面新加的两个链接中都包含“setting”,感觉这行cloud-env的转换和上面的可以去掉。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,我试试看

"Connect success": "连接成功!",
"Connect failed": "连接失败 :(",
"User authentication": "用户身份验证",
"CLOUD-ENVIRONMENT-DESCRIBE": "平台的用户可以将以下开启的云环境对接到平台中来,做为实例或测试实例运行的环境。",
Copy link
Contributor

Choose a reason for hiding this comment

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

像这样的以大写字母作为key的翻译最好把英文也顺便加上来,最后查看具体页面的代码很难知道这里的英文没有翻译。

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

@import '~scss/vars';

.title {
@include title-font($font-size: 12px);
Copy link
Contributor

Choose a reason for hiding this comment

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

页面title在Layout组件传过去了,这里应该是非title样式,应该引用@include normal-font;
title-font是字体加粗的颜色深一点的$N500,normal-font是页面上一般显示描述的字体,字体颜色为$N300.note-font为灰色字体且颜色为$N75的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

@sunnywx sunnywx merged commit 2f824a9 into openpitrix:master Jan 18, 2019
@liiil825 liiil825 mentioned this pull request Jan 22, 2019
2 tasks
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.

3 participants