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

CRM-21312 Fix display of 'Recent Items' when using a bootstrap theme #11138

Merged
merged 2 commits into from
Nov 15, 2017

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Oct 14, 2017

Overview

Before this patch, recent items is messed up on bootstrap themes. Afterwards, it is not messed up!

Before

crm-21312_civi_recentitems_bootstrap_before

After

crm-21312_civi_recentitems_bootstrap_after

Technical Details

Tested on Bartik, Adaptivetheme, bootstrap. Works well on all of them.


@seamuslee001
Copy link
Contributor

@mattwire I'm guessing this isn't relevant to shoreditch is it because it uses its own css or will this impact shoreditch as well?

@seamuslee001
Copy link
Contributor

The code looks fine and simple and i don't think it would hurt anything just would like confirmation on impact on shoreditch first

@eileenmcnaughton
Copy link
Contributor

@guanhuan @jmcclelland - see Shoreditch q

@mukeshcompucorp
Copy link
Contributor

mukeshcompucorp commented Oct 16, 2017

@mattwire Padding 0 is ok but, making overflow:hidden will hide view edit option from list item, can you write counter css which allows to make it visible someway properly?

Before:
test-sidebar-before

After:
test-sidebar-after

Hint: you can use icons to consume less space.

@jmcclelland
Copy link
Contributor

@eileenmcnaughton I think you may have intended to ping jamie novak instead (not sure what his handle is)?

@mattwire mattwire changed the title CRM-21312 Fix display of 'Recent Items' when using a bootstrap theme WIP CRM-21312 Fix display of 'Recent Items' when using a bootstrap theme Oct 22, 2017
@mattwire mattwire force-pushed the CRM-21312_bootstrap_css_fix branch from d5ab334 to e6110f2 Compare October 24, 2017 13:51
@mattwire
Copy link
Contributor Author

mattwire commented Oct 24, 2017

@mukeshcompucorp Ok, take 2. I've wrapped the item name in a separate div so we can set overflow: hidden on that element and not affect the others. This also has the benefit that the hardcoded string length of 25 can be removed and it looks sensible on wider displays.

crm-21312_civi_recentitems_bootstrap_after_new2
crm-21312_civi_recentitems_bootstrap_after_new

@mattwire mattwire changed the title WIP CRM-21312 Fix display of 'Recent Items' when using a bootstrap theme CRM-21312 Fix display of 'Recent Items' when using a bootstrap theme Oct 24, 2017
@eileenmcnaughton
Copy link
Contributor

test this please

@igorpavlov-zz
Copy link

igorpavlov-zz commented Oct 26, 2017

Please amend the overview to specify what is exactly fixed like "Fixed the layout for recent items, ... etc".

Please also amend the screenshots so they show the whole page. If this page supports mobile layout the best thing to do might be providing a GIF where the resizing process is seen.

"Tested on Bartik, Adaptivetheme, bootstrap. Works well on all of them." - this is not Technical Details. Technical details would be a CSS code snippet. What is provided is more something that goes under --- or in a Comment section.

</div>
</li>
<li class="crm-recently-viewed">
<div class="crm-recentview-item">

Choose a reason for hiding this comment

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

I wonder would it be nicer to wrap a DIV into A? In that case the area of click will cover the whole div, not just a caption (not a change request but a thought)

@eileenmcnaughton
Copy link
Contributor

Hi guys - just trying to understand the degree of testedness / agreedness of this - I don't want to get blocked by the semantics of filing in the template - it seems agreed this is an improvement in some cases but I'm not sure if we have agreement this won't cause regressions in others.

@mattwire
Copy link
Contributor Author

mattwire commented Nov 8, 2017

@mukeshcompucorp Can you confirm you are happy with this now?

@mlutfy
Copy link
Member

mlutfy commented Nov 15, 2017

I tested on a CiviCRM with an existing Bootstrap theme (which had tweaked the CSS in a not-very-clean way, so I wanted to see if it would cause problems) and it seems OK:

civicrm-recent-items-bootstrap

So I would be in favour of merging this - risk of breakage for existing themes seems minimal.

@eileenmcnaughton
Copy link
Contributor

I second @mlutfy comments & am merging - the main concern was to get Compucorp input as to whether this had any negative effects on the Shoreditch theme. An impact was identified & resolved & other comments seem to be about stylistic nuances & administrative aspects. Based on @mlutfy testing I will merge - but I suggest @guanhuan that Compucorp make a point of testing the rc / master branch

@eileenmcnaughton eileenmcnaughton merged commit 587fdbc into civicrm:master Nov 15, 2017
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
…s_fix

CRM-21312 Fix display of 'Recent Items' when using a bootstrap theme
@mattwire
Copy link
Contributor Author

@lsmithgo I'm guessing that is an issue specific to whatever theme / theme customisation you are using. Works fine on the standard themes (just checked again).

@lsmithgo
Copy link
Contributor

@mattwire I'm using the standard theme (to the best of my knowledge). Problem only shows when the item title is long, eg when showing a Participant registered to an Event. See the screenshot.
capture

@mattwire
Copy link
Contributor Author

@lsmithgo Looks like a Joomla specific issue then... I suggest you open an issue at https://lab.civicrm.org/dev/core/issues making it clear it only affects Joomla, and ideally try and work out what the best fix for this might be (that doesn't stop it working properly on other CMS).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants