-
Notifications
You must be signed in to change notification settings - Fork 36
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
ACMS-1989: Update admin theme in Acquia CMS to Gin. #1649
Conversation
d20b107
to
f0a062e
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.
Added a minor suggestion rest everything looks good
741c60f
to
26e4f95
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.
@rajeshreeputra - I have requested some changes, please take a look.
882df23
to
9cbb267
Compare
9cbb267
to
c9ba23d
Compare
c9ba23d
to
e23fb63
Compare
9d9e90d
to
5b65560
Compare
ec88a25
to
980270e
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.
@rajeshreeputra - I can see some changes which are not related to work of this ticket, like refactoring, I would suggest to do this in a separate ticket and lets add only changes which are related to this ticket.
Also don't include patch for the work that is not merged, rather provide steps to pull that in during verification.
@vishalkhode1 @vipin-mittal-acquia - what's your though on this?
980270e
to
7fcb19e
Compare
Removed the changes not related to work of this ticket.
The patch has been added to the root composer.json file. In our CI environment, the site is installed using the starterkit, and it's important to note that there have been changes related to the Gin theme, although these changes have not been released yet. This means that our CI process will encounter failures because the starterkit version 1.1.5 now requires Acquia Claro to be installed in order to set up the site and run tests. However, in the pull request, Acquia Claro has been removed, causing the CI failures. Hence it is required to be added in root composer.json file. |
ok, then lets create a ticket to remove this patch when we do the release for this changes. |
Motivation
Fixes #ACMS-1989
Proposed changes
This PR is created to check if there any test failure with Gin theme.
Alternatives considered
This can be closed without any further action once the issue identification is completed, or it can be used, and further changes can be made here directly.
Testing steps
NA
Merge requirements