Skip to content

move select2 styles to core so it's universal, not only for tags#1789

Merged
MorrisJobke merged 2 commits into
masterfrom
select2-fixes
Oct 18, 2016
Merged

move select2 styles to core so it's universal, not only for tags#1789
MorrisJobke merged 2 commits into
masterfrom
select2-fixes

Conversation

@jancborchardt

@jancborchardt jancborchardt commented Oct 18, 2016

Copy link
Copy Markdown
Member

Before & after: Much more breathing space, less complexity and lines, nice Nextcloud style, and proper connection to the input field:
capture du 2016-10-18 19-46-21capture du 2016-10-18 19-54-38

Please review @nextcloud/designers @ErikPel @nextcloud/javascript you can test it with:

The one problem which still occurs (also with old version) is that the dropdown is about 3 pixels too wide on the right. Any idea @nextcloud/javascript? (NOT A BLOCKER)

@jancborchardt jancborchardt added enhancement design Design, UI, UX, etc. 3. to review Waiting for reviews labels Oct 18, 2016
@jancborchardt jancborchardt added this to the Nextcloud 11.0 milestone Oct 18, 2016
@mention-bot

Copy link
Copy Markdown

@jancborchardt, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @vincchan and @Henni to be potential reviewers.

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@LukasReschke

LukasReschke commented Oct 18, 2016

Copy link
Copy Markdown
Member

Works locally. Looks nice! 🚀

LGTM

@skjnldsv

skjnldsv commented Oct 18, 2016

Copy link
Copy Markdown
Member

@jancborchardt it's because the dropdown uses the width of .select2-choices
And it has margin which sums up to 333px instead of what you wanted.

Comment thread core/css/inputs.css Outdated
box-sizing: content-box !important;
border-radius: 3px !important;
border: 1px solid #ddd !important;
margin: 3px 3px 3px 0 !important;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here this is you issue @jancborchardt

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed while keeping the margin on the container, thank you! :)

@skjnldsv

Copy link
Copy Markdown
Member

Ps: there's a lot of !important overriding. Is this really necessary? Shouldn't we load the jquery theme before instead?

@jancborchardt

Copy link
Copy Markdown
Member Author

Ps: there's a lot of !important overriding. Is this really necessary? Shouldn't we load the jquery theme before instead?

I agree. @MorrisJobke @LukasReschke how can we change the order here? The inputs.css should be one of the last loaded.

@MorrisJobke

Copy link
Copy Markdown
Member

I agree. @MorrisJobke @LukasReschke how can we change the order here? The inputs.css should be one of the last loaded.

Crash course for @jancborchardt how to do this:

  1. use the search tool of your choice to search for where the CSS is imported
  2. change the order there
  3. don't always out source this to other people - even if you think this would be nice because they know where - we are no weird dudes who have all the files and lines in our head - we also always use the search tools

@MorrisJobke

Copy link
Copy Markdown
Member

don't always out source this to other people - even if you think this would be nice because they know where - we are no weird dudes who have all the files and lines in our head - we also always use the search tools

OC_Util::addStyle("inputs",null,true);

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@skjnldsv

skjnldsv commented Oct 18, 2016

Copy link
Copy Markdown
Member

The order is reversed when implementing. So in the file @MorrisJobke pointed (thanks 😉 ) you need to pu inputs BEFORE jquery! 😁

capture d ecran_2016-10-18_20-28-54

@jancborchardt

Copy link
Copy Markdown
Member Author

@MorrisJobke I searched for it, but since it was called legacy I assumed it’s not used anymore. There was another file which looked like it’s done there and it seemed like it just loaded all the files from the CSS directory. So thanks for helping.

@jancborchardt

Copy link
Copy Markdown
Member Author

Experimented with removing all the !importants and with changing the order – doesn’t work. If anyone can make it work I’m happy for enhancements, but I’d say let’s get it in.

@skjnldsv

skjnldsv commented Oct 18, 2016

Copy link
Copy Markdown
Member

This is strange! 😕
Okay, this is because select2 is coming after. Its from the app systemtags.

@skjnldsv

Copy link
Copy Markdown
Member

@jancborchardt maybe we should add this into the systemtags apps? Since this is only used here?

@MorrisJobke

Copy link
Copy Markdown
Member

The order is reversed when implementing. So in the file @MorrisJobke pointed (thanks 😉 ) you need to pu inputs BEFORE jquery! 😁

Correct - this is a bit weird, but you figured it out 😉

@MorrisJobke

Copy link
Copy Markdown
Member

👍

@MorrisJobke MorrisJobke merged commit e115bf9 into master Oct 18, 2016
@MorrisJobke MorrisJobke deleted the select2-fixes branch October 18, 2016 20:05
@jancborchardt

Copy link
Copy Markdown
Member Author

maybe we should add this into the systemtags apps? Since this is only used here?

No – as said above it’s also used elsewhere like app mgmt and the spreed app. :) That’s the point of this PR, to make the select2 style adjustments more general.

@skjnldsv

Copy link
Copy Markdown
Member

Okay! :)
So maybe we should add the select2 style injection in core and not systemtags then :p

@jancborchardt

Copy link
Copy Markdown
Member Author

@skjnldsv that’s what I did here, moved most of the select2 styles from systemtag.css to core/css/inputs.css ;)

@skjnldsv

Copy link
Copy Markdown
Member

Yes, but I mean the addStyle function from

OC_Util::addStyle("inputs",null,true);

Would be better than loading it from here https://github.com/nextcloud/server/blob/master/apps/systemtags/appinfo/app.php#L35

@jancborchardt

Copy link
Copy Markdown
Member Author

@skjnldsv good point! Do you want to do the change in a pull request? :)

@skjnldsv

skjnldsv commented Oct 19, 2016

Copy link
Copy Markdown
Member

I'm busy with the scss! 😁
Do it !

demandingfemaleflyingfish-size_restricted

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 Design, UI, UX, etc. enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants