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

Switch to SCSS #1805

Closed
wants to merge 25 commits into from
Closed

Switch to SCSS #1805

wants to merge 25 commits into from

Conversation

skjnldsv
Copy link
Member

This is related to #1786
Please only use this pr to discuss issues or comments about its commits.

For arguing and/or ask question, go to the issue.

@skjnldsv skjnldsv added enhancement help wanted design Design, UI, UX, etc. 2. developing Work in progress labels Oct 19, 2016
@skjnldsv skjnldsv added this to the Nextcloud 11.0 milestone Oct 19, 2016
@skjnldsv skjnldsv self-assigned this Oct 19, 2016
@mention-bot
Copy link

@skjnldsv, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @bartv2 and @owncloud-bot to be potential reviewers.

@@ -0,0 +1,113 @@
<?php
/**
* @copyright Copyright (c) 2016, nextCloud, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Nope, the copyright belongs to you ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, if you insist! :3

}
}

?>
Copy link
Member

Choose a reason for hiding this comment

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

No need to close this here - just remove this. This also reduces possible issues with whitespace after the closing tag that cause a different output to the browser.

@rullzer
Copy link
Member

rullzer commented Oct 19, 2016

Please use the AppData to store stuff. Don't go writing into folders.

@LukasReschke
Copy link
Member

Please use the AppData to store stuff. Don't go writing into folders.

So store in AppData and use PHP to serve and set proper caching headers? We can't assume that assets is writable at the moment :)

@rullzer
Copy link
Member

rullzer commented Oct 19, 2016

@LukasReschke exactly

@skjnldsv
Copy link
Member Author

Sure! No problem! 😃

*
* @author John Molakvoæ <skjnldsv@protonmail.com>
*
* @license AGPL-3.0
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course! 😃

width: 90%;
}

.ui-autocomplete {
Copy link
Member Author

Choose a reason for hiding this comment

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

@jancborchardt why don't we have a global definition for this? Shouldn't the dropdown use select2? :)

Copy link
Member

Choose a reason for hiding this comment

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

Opened a separate issue about that at #1831 ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

🎩

@jancborchardt
Copy link
Member

Btw @skjnldsv while we rewrite the whole file to new syntax and all the author info is lost, we should probably add the copyright headers at this point to each file. :)

@skjnldsv skjnldsv mentioned this pull request Oct 20, 2016
17 tasks
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 20, 2016

we should probably add the copyright headers at this point to each file. :)

NO! NO COPIRIT FOR U!

Yeah, I saw that. I will readd all the copyright after with a unified header so it looks good 😉

*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
Copy link
Member

Choose a reason for hiding this comment

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

@nickvergessen why the extra space here?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

😆

@skjnldsv skjnldsv force-pushed the scss-switch branch 2 times, most recently from ed745d2 to d7902cb Compare October 22, 2016 03:57
skjnldsv and others added 7 commits November 2, 2016 10:43
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
…o core

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@rullzer
Copy link
Member

rullzer commented Nov 8, 2016

@skjnldsv what is the progress here?

Any chance of breaking it up into smaller PR's As reviewing 1800 lines is kind of a pain.

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 8, 2016

No chance at all! 😂 😂
I know this is a lot of work, but imagine what I had to do 😆

The best way to check this pr i think will be to check the validity of the generated css, and then explore your nextcloud installation looking for mistakes (which I already did and try to check everytime I make a small change) 😕

Concerning the progress, It's going fast I think. I'm almost done converting, and the next big thing will be the appdata integration (see original issue)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@juliusknorr
Copy link
Member

@rullzer @skjnldsv https://codeascraft.com/2015/02/02/transitioning-to-scss-at-scale/#scss-diff might be interesting to check if the resulting css code has fundamental changes.

@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 8, 2016

@juliushaertl Indeed! I wonder if I shouldn't listen to @rullzer and create separate commits.
One with the scss implementation and a basic conversion, and multiples seconds with each one a simplification. I should have done that since the beginning! 😢

@juliusknorr
Copy link
Member

@skjnldsv Creating separate PRs should not be that hard, just create a new branch and cherry pick the commits you need from this one.

@MorrisJobke
Copy link
Member

@skjnldsv Is this still needed?

@skjnldsv
Copy link
Member Author

skjnldsv commented Jan 6, 2017

@MorrisJobke probably not!

@skjnldsv skjnldsv closed this Jan 6, 2017
@rullzer rullzer deleted the scss-switch branch January 6, 2017 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants