Skip to content

Conversation

@Miliooo
Copy link
Contributor

@Miliooo Miliooo commented Oct 17, 2013

I've updated the included template with bootstrap3. This is not an attempt to make a kick ass design. Authors would still need to overwrite these templates. Also the forms are not styled yet.

Another update is in the translations file. There is an unneeded (and from my experience not working) string -message_info so i deleted that one. I only did that for the english version for now.

New template

rsz_1inbox
rsz_message

Old template

rsz_inbox_old
rsz_message_old

@merk
Copy link
Member

merk commented Oct 17, 2013

Sorry, I'm not willing to merge something with a dependency on a layout library. You can always provide a documentation page on adding something like this instead.

@merk merk closed this Oct 17, 2013
@merk
Copy link
Member

merk commented Oct 17, 2013

Additionally, please send separate pull requests through for each change, grouping them together makes it impossible to talk about each change individually.

@Miliooo
Copy link
Contributor Author

Miliooo commented Oct 17, 2013

I respectfully disagree with you on creating a dependency. You are forced to overwrite layout.html.twig no matter what or you'll end up with a site with no css. And the little bit of markup that I did on the other files... I would argue that you also HAVE to overwrite those too because how on earth would you create something that would even resemble a design if you don't modify those.

I see one problem though and that is

     <link rel="stylesheet" href="//netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap.min.css">

would require you to be online. It wouldn't be a disaster if you were not, it would just look as bad as it does now. An option would be to add the css locally.

That being said, if you'd really prefer a barebone layout example (which the users will have to overwrite too) then I respect that. The main reason I made this as a pull request is that I want to add flashbag messages to actions (configurable to be disabled...) and I think that a basic design that shows those actions would help there.

I think I found two bugs in the current layout:

  • the sentbox title is messenger and should be 'sent'
  • -there is an unneeded translation string because we already translate them seperatly. Update thread.html.twig #180

I'll recheck and submit separte pr if needed soonish

@merk
Copy link
Member

merk commented Oct 17, 2013

I'm not sure about the translation: what if a language reverses the order of the words? I dont see an issue with translating sentences instead of just words.

As for the layout, I'm happy for it to be improved slightly but (as per all other bundles FOS provide) we dont provide much in the way of styling by choice.

As for the flashbag stuff, again its something that only some users will want and would be better if we make it easy for it to be added, but not add it by default.

@Miliooo
Copy link
Contributor Author

Miliooo commented Oct 17, 2013

About those flashes:

Now when you delete a thread, you'll get send back to your inbox with no notification at all.
Also when you create a thread, yeah you'll get redirected to the message, but there is no clear indication that the message is sent.

I think many sites show flash messages when the user does an action. And those who don't probably don't have their layouts designed to show flashes. So it wouldn't bother them. That being said making it optional was something I already assumed would have to be the case to even have a chance to get this in core. And I understand that.

About those current bugs:
I submitted two pull requests who fixes those

About only providing a bare bone design:
I understand but I still don't think I added a dependency.

@stof
Copy link
Member

stof commented Oct 17, 2013

Well, you have introduced a dependency to bootstrap

@Miliooo Miliooo deleted the bootstrap branch October 22, 2013 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants