Build css from scss sources in client ui build

[pbankonier]

Description

The following things are done in this PR:

• Add the compiling of the scss sources to css to the maven build for the client ui. I used this (https://github.com/eirslett/frontend-maven-plugin) maven plugin for that.

• This was required to remove the css files from the project. This is necessary, because it happened multiple times that the css files were modified directly instead of editing and then compiling the scss files, which results in a mismatch between the scss src and the compiled css.
  e.g. https://github.com/apache/cloudstack/pull/3240/files
  I moved these missing changes into the corresponding scss files here.

• This (compiling scss to css during the build) is the currently preferred way of handling these frontend resources.

• This changes are not affecting any functionality nor are a breaking change.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

Deployed this to our test environment and tested functionality.

[svenvogel]

@anuragaw @shwstppr can you take a look?

[rohityadavcloud]

Can you explain why move the ui directory to client?

[pbankonier]

Can you explain why move the ui directory to client?

I found this location more appropriate, because the frontend resources in this ui folder are the main resources for the client ui module, so they fit better in there instead of the root directory of the project. Also they now get more intensively used by this build so I think it might be a good idea to keep them together. They don't get used by any other module.

[rohityadavcloud]

In the present form renaming ui folder just because it feels like may not be encouraged because it will cause conflicts for so many existing PRs and will cause porting and forward merging issues. Let's visit this in future, at least after 4.13.

However, if you want the PR go into 4.13 please remove the renaming changes.

[anuragaw]

Agree with @rhtyd. I would prefer to postpone any major restructuring to after 4.13 as well unless it's an urgent necessity. Please note cut dates for 4.13 are out and major changes for 4.13 should be in debate or close to merge by now already.

[svenvogel]

@rhtyd @anuragaw yes we move the ui folder back in the old place and put it on the list after 4.13 release so we have no porting problems. the commit will come in the next days. 👍

[rohityadavcloud]

@svenvogel I'll readded this PR to 4.13.0.0 milestone on your remark. Keep us posted, thanks.

[svenvogel]

@rhtyd ping we moved the files back in the old location. it should look okay now.

[rohityadavcloud]

Thanks @svenvogel but I still see several large changes and file renames and I'm not sure I understand what is intended to be here. I can make out that it introduces a new gulp/npm dependency during build.

Please have a look at:
#3444 (comment)

Copied from above:

 there have been several css/UI related PRs proposed recently, can you start a discussion thread on dev@ to explain what are the goals and refactorings you're trying to do. I'm personally not sure where the UI changes are heading and would like to learn more.

Typically any framework/component related changes and refactorings may have a design document to capture this that is shared with the dev community for review and discussion. Any proposals and discussions are welcome on dev@. Thanks.

[webermaximilian92]

Hey @rhtyd
Please show us some examples of "large changes" and "file renames".
May I ask you to be a bit more specific and work with examples please?
Does my @dev mail list post helps you to understand what we are planing to do?

[rohityadavcloud]

@webermaximilian92 I see some large css files removed, I don't understand if we're removing them how does that affect our build/usage?

@pbankonier can you fix the conflict GitHub is complaining about?

[pbankonier]

@rhtyd That these css files get finally removed is just a consequence from this change #3307. The css files should not be edited nonetheless right now, because they get build from the scss sources. So the css files should have been removed already in that PR. Everyone who wants to make a styling change should edit the scss files and then build the css from them. Normally you would not keep the css files in the repo, because have redundant information regarding the scss files, so when someone changes one without the other we will get an inconsistent state of these files, which results in the necessity of someone keeping them in sync manually.
So to prevent this it is important to get this change merged, which compiles the scss files to css during the build of the module and removes the responsibility for keeping these files in sync from the devs. There is no impact in the usage.
The only alternative would be to remove the scss files again, but this would result in keeping these massive css files which are difficult to maintain.

[rohityadavcloud]

Thanks for explaining @pbankonier I would have asked for a complete design document for all sorts of UI changes that are coming because they have long term impact on all developers including build and CI systems. I'm sure there are others in the community who're unable to track the intent spread across a couple of related PRs.

I was not aware of the PR #3307 and was not tracking it as it was not tagged against a milestone (just added one because it's already merged). Generally reviewers, RMs and co-RMs working towards a release/milestone may not track PRs that bring wide amount of changes or break several existing PRs because it will be a lot of work to get authors of existing PRs which may get merge conflict if a PR like this would be merged. We have about 90-100 open items on 4.13.0.0 milestone and many of which have UI/css changes or are UI issues that may require fixes in css files. Due to this, I have moved milestone of this PR to 4.14, hope I've your support and understanding.

[rohityadavcloud]

-0 I tried to review the diff, but I don't know gulp and scss enough to be able to review the changes. I can get around with the UI and css changes, I think this change will make it slightly harder and also since the UI lacks tests (both functional/integration and unit) it will be harder to regression test.

[borisstoyanov]

@pbankonier can you address the merge conflicts pls

[rohityadavcloud]

@pbankonier @svenvogel - do you think we can close this now that we have Primate?

[rohityadavcloud]

@pbankonier @svenvogel - with primate in, kindly close this. Let's move all major UI development work in Primate. Thanks.

[svenvogel]

@rhtyd closed in favor of cloudstack primate development https://github.com/apache/cloudstack-primate/