-
Notifications
You must be signed in to change notification settings - Fork 371
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
[ui-configuration] Convert the Configuration page within Administrator Server in ReactJS #3848
base: master
Are you sure you want to change the base?
Conversation
27e1657
to
8c610a0
Compare
2898b6c
to
68d5569
Compare
1c48c1e
to
492ee86
Compare
59c3ba6
to
9bd1725
Compare
9ea914b
to
2a8e4be
Compare
2e71ea0
to
a4a26cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, please see feedback.
made some comments about how to name and structure the html classes (css) in the first files, but the comments apply to all files in the PR.
desktop/core/src/desktop/js/apps/admin/Configuration/ConfigurationTab.test.tsx
Outdated
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/admin/Configuration/ConfigurationTab.tsx
Outdated
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/admin/Configuration/ConfigurationTab.tsx
Outdated
Show resolved
Hide resolved
desktop/core/src/desktop/js/apps/admin/Configuration/ConfigurationTab.tsx
Outdated
Show resolved
Hide resolved
1d8dae6
to
26fea9f
Compare
Done till before bringing config in alignment styling changes fixing checks newly added WIP WIP WIP Renaming Linting fixed WIP WIP WIP WIP WIP WIP WIP WIP hey WIP WIP WIP WIP wIP WIP WIP
26fea9f
to
236c80c
Compare
bf8d745
to
0445e52
Compare
|
||
.config__section-header { | ||
color: $fluidx-gray-700; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation, do a style lint and fix indentation here.
padding: 4px 0 8px 0; | ||
} | ||
|
||
padding: 16px 0 8px 16px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these styles(padding, bg-color,border) belongs to the config__main-item class add it on top itself instead of adding before closing of this class
} | ||
|
||
.config__main-item .config__child-item .config--last-heading { | ||
font-size: $font-size-base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add it on line 36, same class
font-size: $font-size-base; | ||
color: $fluidx-black; | ||
font-weight: 400; | ||
margin-left: 40px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
font-size and styles looks same for all child elements, we don't need to repeat them include these styles in parent class
const filterConfig = ( | ||
config: AdminConfigValue, | ||
lowerCaseFilter: string | ||
): AdminConfigValue | undefined => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is undefined required here?
What changes were proposed in this pull request?
Shifting the entire old code for the Configuration tab (page) which is present in Administrator server as a ReactJS component.
How was this patch tested?
Manual checking. Unit tests for all the components added.
Old Configuration page:
Updated Configuration page:
Please review Hue Contributing Guide before opening a pull request.