-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
AMBARI-26343 React Implementation: Authentication Components #3963
base: frontend-refactor
Are you sure you want to change the base?
Conversation
6d05153
to
268e795
Compare
Ambari-Web React Implementation: Authentication Components
268e795
to
40b2dcb
Compare
Thanks @zRains cc |
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.
@chenyuan99 thanks for the PR. The changes in this PR are in the ambari-admin
folder. However, AMBARI-26343 is for ambari-web
. Please check https://issues.apache.org/jira/browse/AMBARI-26163 [Epic for Ambari Admin UI modernization]. Can you please create an issue there and link it in this PR?
Hi @nikita15p , yes it is for ambari-web, and the login component is part of the ambari-web, but i dont see there is merites to create three seperate react front end projects, react router will take care of the rest. |
@chenyuan99 My understanding is that these frontend projects are being developed in a merged manner, similar to a Monorepo? |
yes you are right, since react is a component based frontend framework, it is a good idea keep the components in the same place to make the components reusable |
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.
The current changes include mock code and lack relevant unit tests. Additionally, there are differences in the server-side responses, meaning that ambari-server will need corresponding changes @JiaLiangC. We can merge this first to facilitate further development.
)} | ||
<div | ||
className="py-3 d-flex justify-content-center sidebar-collapse icon-primary" | ||
style={{ background: "#313d54" }} |
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.
I think using bg-[#313d54] would be a better choice. Reducing inline styles helps with managing and modifying styles more efficiently. Alternatively, you could define it as a variable in the Tailwind config file.
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.
will update that
Reference: Sharing history on the design plan of modernisation work #3967 (comment) |
@chenyuan99, seems this is a design change and a diversion from existing implementation. Separation of modules exists since inception and re-birth of the project. Since this is a design change can you kindly share a document proposing and justifying the same. |
Ambari-Web React Implementation: Authentication Components
What changes were proposed in this pull request?
fix AMBARI-26343
How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests)
(If this patch involves UI changes, please attach a screen-shot; otherwise, remove this)
Please review Ambari Contributing Guide before opening a pull request.