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

Move to SCSS [was: App to css guidelines] #613

Merged
merged 38 commits into from
Jan 11, 2018
Merged

Conversation

skjnldsv
Copy link
Member

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added 2. developing Work in progress design Related to design, interface, interaction design, UX, etc. skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills priority labels Sep 23, 2017
@skjnldsv skjnldsv self-assigned this Sep 23, 2017
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>
@georgehrke
Copy link
Member

Will these changes be compatible with Nextcloud 11 and 12 too?

@skjnldsv
Copy link
Member Author

For now compatible 13 only. We'll have to decide if we want to implement a fix until the last supported version is 13. Or something else! :)

…/bgs

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

@georgehrke I added a lots of stuff!
@tcitworld @raghunayyar Could you test it please?
Please list anything you find and I will fix it or explain the choice I made! :)

Also: please test it under nc12. This pr will allow the app to only nc12 and higher, this is unfortunate but all the other apps are doing the same to enjoy the scss unification! :)

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

@skjnldsv Is this ready to review?

@skjnldsv
Copy link
Member Author

@georgehrke sorry, forgot that :p
Almost!

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 4, 2017
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 4, 2017

@georgehrke I think we're good here. :)

@georgehrke
Copy link
Member

I created a stable1.5 branch, because this breaks compatibility with Nextcloud 11, which is still supported.

@skjnldsv You said this works with Nextcloud 12, right?

@skjnldsv
Copy link
Member Author

No, a specific patch will still be needed.

@georgehrke
Copy link
Member

georgehrke commented Nov 14, 2017

  • The new / edit dialogs in the sidebar look a little weird:

0afa7d3e-b0ca-4b9e-80b4-8356127e05a7
164b9d51-00a4-441d-b3e9-88e81b1614bd
360b2d9e-1842-41ba-9bb2-e16c87e4bff7

  • Furthermore the color-picker overlaps with the first item in the calendar-list

Color-picker issue considered unimportant for now. Should be removed in the long run and the new calendar should just be assigned a color that is not in use so far.

@georgehrke
Copy link
Member

@skjnldsv Can you check this please :)

@skjnldsv
Copy link
Member Author

Hum, in the end, it could be a good thing to experiment with the color picker on a popover? :)

@georgehrke
Copy link
Member

georgehrke commented Dec 19, 2017

9e4e736f-5411-42fb-8bde-1fdc6faefa7b

  • The dropdown menu is hidden behind the new subscription input

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
Copy link
Member

georgehrke commented Dec 19, 2017

  • The sharing opacity thing doesn't work for me. All sharing icons are the same, shared or not shared

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke mentioned this pull request Jan 9, 2018
10 tasks
…ubscription form

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
…icon

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke dismissed their stale review January 10, 2018 19:34

fixed all issues i found

@georgehrke
Copy link
Member

@skjnldsv @tcitworld I fixed all the issues that i found. Can you double check? :)
I would like to merge this soon

@tcitworld
Copy link
Member

Will have a look this afternoon.

@skjnldsv
Copy link
Member Author

Amazing work @georgehrke Works perfectly! 😍 🎉

@georgehrke
Copy link
Member

@tcitworld Is it okay if i go ahead and merge this?

Given the release schedule of the server i would like to get this in fairly soon ;)

@tcitworld
Copy link
Member

All I saw was this issue :
The nc logo is not displayed on the public calendar page
selection_161

@georgehrke
Copy link
Member

That seems related to this: #571 :)

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld
Copy link
Member

Fixed. Good to go.

@georgehrke
Copy link
Member

awesome, let's get this in :)

@georgehrke georgehrke merged commit 17e6951 into master Jan 11, 2018
@georgehrke georgehrke deleted the app-css-guidelines branch January 11, 2018 19:11
@tcitworld
Copy link
Member

Did realize I didn't test this on nc12 though.

@skjnldsv
Copy link
Member Author

@tcitworld if we want to keep it ok for 12, it's an easy fix :)
@georgehrke do you both want to keep 12 + 13 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Related to design, interface, interaction design, UX, etc. priority skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants