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

[4.0] Group toolbar buttons [com-content -> articles] #21247

Closed
wants to merge 22 commits into from

Conversation

ciar4n
Copy link
Contributor

@ciar4n ciar4n commented Jul 24, 2018

Pull Request for Issue joomla/40-backend-template#348 .

Summary of Changes

Groups com_content -> articles toolbar buttons

Credit to @asika32764 for adding #19670

Testing Instructions

Apply PR and navigate to com_content -> articles.

Before PR

image

After PR

image

image

Documentation Changes Required

Yes

@ciar4n ciar4n requested a review from brianteeman as a code owner July 24, 2018 13:28
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Jul 24, 2018
Copy link
Contributor

@brianteeman brianteeman left a comment

Choose a reason for hiding this comment

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

approve new language string

@brianteeman
Copy link
Contributor

This loses the work that @Fedik did to disable buttons

@ciar4n
Copy link
Contributor Author

ciar4n commented Jul 24, 2018

@brianteeman Would you have the number on that PR?

@ciar4n
Copy link
Contributor Author

ciar4n commented Jul 24, 2018

Any suggestions for the 'Change Status' icon and color?

@brianteeman
Copy link
Contributor

I think the pr was #20650

You will need to get the a11y team to review this so that the dropdown is described and announced correctly

@ciar4n
Copy link
Contributor Author

ciar4n commented Jul 24, 2018

You will need to get the a11y team to review this so that the dropdown is described and announced correctly

@joomla/joomla-accessibility-team-jat Please review.

@ghost
Copy link

ghost commented Jul 24, 2018

I have tested this item ✅ successfully on 47fc7c1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21247.

@ciar4n
Copy link
Contributor Author

ciar4n commented Jul 24, 2018

This loses the work that @Fedik did to disable buttons

@Fedik As JS is not really my forte, I'll ask you for some guidance on this.

@chmst
Copy link
Contributor

chmst commented Jul 24, 2018

I would like to remember my PR and show only buttons if they can do someting . https://issues.joomla.org/tracker/joomla-cms/20559

@dgrammatiko
Copy link
Contributor

@ciar4n are you using Bootstrap's dropdown here?

@ghost
Copy link

ghost commented Jul 24, 2018

@chmst

show only buttons if they can do someting

+1

@ciar4n
Copy link
Contributor Author

ciar4n commented Jul 24, 2018

@ciar4n are you using Bootstrap's dropdown here?

Yes. I think doing anything else is outside of the scope of this PR. For now lets consider that grouping buttons is the way to go and if so, how best to group them,

@brianteeman
Copy link
Contributor

no - lets wait a year for a custom element - that solves everything everywhere

@dgrammatiko
Copy link
Contributor

@brianteeman you know what I'll revert all the custom elements work. Back to jQuery bootstrap seems what people want! So let me please you and good luck with Joomla 4...

@wilsonge
Copy link
Contributor

Please stop fighting. Ciaran is correct that this is not the place for the custom elements discussion - he's using the api for grouping buttons. Whether that uses Bootstrap or Custom Elements is a totally different issue/PR. (To an extent so is the accessibility of that dropdown as it's not just going to be used here - but across many extensions for differing groupings - although of course if we can sort that here all the better!).

However ensuring that we don't regress the grey'ing out of buttons when no items are selected is important and should be considered as part of this issue

@Fedik
Copy link
Member

Fedik commented Jul 26, 2018

the question here, what should be "grayed" (disabled), the "main" button of the group, or the elements in it?

the elements not grayed, because in the group they are "anchors" <a> not <button>, if I right remember,
I try to check more next weekend

@brianteeman
Copy link
Contributor

@Fedik As none of the "change status" options will be enabled unless an item is selected then I would suggest that it should be the main button that is disabled

@chmst
Copy link
Contributor

chmst commented Jul 26, 2018

My2Cent: The graying needs a designer. As it is now, I always try to clean my glasses. Maybe gray could be really gray?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21247.

@brianteeman
Copy link
Contributor

Or even grey 😂

@ghazal
Copy link
Contributor

ghazal commented Jul 27, 2018

I have tested this item ✅ successfully on 47fc7c1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21247.

@joomla-cms-bot joomla-cms-bot removed Language Change This is for Translators PR-4.0-dev labels Jul 27, 2018
@ghost
Copy link

ghost commented Jul 27, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 27, 2018
@laoneo laoneo added this to the Joomla 4.0 milestone Jul 27, 2018
@wilsonge
Copy link
Contributor

Please remove the RTC as per discussions above here #21247 (comment) it is not ready to be merged yet

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 14, 2018

@wilsonge if you can merge this, i can look at doing similar in other views

@@ -6,7 +6,7 @@
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Joomla: unsupported PHP version</title><!-- Sets the page title, IMPORTANT DO NOT REMOVE -->
<style>body{margin:0;padding:0;font:14px / 18px sans-serif;color:#555;background-color:transparent}html{background:#f1f1f1;background:-moz-radial-gradient(center,ellipse cover,rgba(241,241,241,1) 0,rgba(58,146,200,1) 100%);background:-webkit-radial-gradient(center,ellipse cover,rgba(241,241,241,1) 0,rgba(58,146,200,1) 100%);background:radial-gradient(ellipse at center,rgba(241,241,241,1) 0,rgba(58,146,200,1) 100%);background-repeat:no-repeat;background-attachment:fixed}ol,ul{padding:0;margin:0;list-style:none}a{color:#0084b4;text-decoration:none;outline:0}a:hover,a:focus{text-decoration:underline}p a{line-height:inherit}.container{display:flex;flex-direction:column;justify-content:center;align-items:center;position:relative;margin:0 auto;width:100%;height:100vh;overflow:hidden}.alert-main{display:block;position:relative;background:#fff;border:1px solid rgba(0,0,0,0.1);border-radius:5px;padding:20px 60px;margin:0 20px;box-shadow:0 0 10px rgba(0,0,0,0.05)}svg{position:absolute;bottom:-120px;right:-70px;width:400px;transform:rotate(10deg);z-index:-1}h1,p{position:relative;z-index:10;text-align:center;text-rendering:optimizeLegibility}h1{margin:18px 0 0;font-size:40px;font-weight:200;line-height:1;text-shadow:0 1px 2px rgba(0,0,0,.2)}p,label{margin:10px 0 20px;font-size:18px;font-weight:300;line-height:25px;color:#777}p a{font-weight:bold;color:#1c3d5c}.link-help{padding:.4rem .85rem;font-size:1rem;font-weight:normal;border-radius:.25rem;text-decoration:none;background-color:#f5f5f5;border:1px solid rgba(0,0,0,0.1)}.link-help:hover{background-color:#eee;text-decoration:none}.footer{margin:8px 20px;text-align:left;font-size:11px}.footer ul{margin-bottom:5px}.footer li{display:inline;margin:0 5px;line-height:20px}.footer li,.footer a{color:#1c3d5c}.footer a:hover{color:#59b0ff}.links{display:block;text-align:center;margin-top:4rem;margin-left:auto;margin-right:auto;font-size:1rem}.links li{display:inline-block;margin-top:20px}@media screen and (max-width:480px){.container{height:auto;padding-top:20px;padding-bottom:20px}h1{font-size:30px}.link-help{white-space:nowrap}}</style><!-- Sets the page styling, IMPORTANT DO NOT REMOVE -->
<script>window.errorLocale = {"en-GB":{"language":"English GB","header":"Sorry, your PHP version is not supported","text1":"Your host needs to use PHP version {{phpversion}} or newer to run this version of Joomla","help-url-text":"Help me resolve this"},"en-US":{"language":"English US","header":"Sorry, your PHP version is not supported","text1":"Your host needs to use PHP version {{phpversion}} or newer to run this version of Joomla","help-url-text":"Help me resolve this"},}</script><!-- Sets the content of the translated text, IMPORTANT DO NOT REMOVE -->
<script>window.errorLocale = {"c:\xampp\htdocs\joomla-4-dev\installation\language\en-GB\en-GB":{"language":"English GB","header":"Sorry, your PHP version is not supported","text1":"Your host needs to use PHP version {{phpversion}} or newer to run this version of Joomla","help-url-text":"Help me resolve this"},"c:\xampp\htdocs\joomla-4-dev\installation\language\en-US\en-US":{"language":"English US","header":"Sorry, your PHP version is not supported","text1":"Your host needs to use PHP version {{phpversion}} or newer to run this version of Joomla","help-url-text":"Help me resolve this"},}</script><!-- Sets the content of the translated text, IMPORTANT DO NOT REMOVE -->
Copy link
Contributor

Choose a reason for hiding this comment

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

@dgrammatiko this looks like a windows build issue if i ever saw one :D

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -928,7 +928,7 @@ public function getItem($pk = null)

// Convert to the CMSObject before adding other data.
$properties = $table->getProperties(1);
$item = ArrayHelper::toObject($properties, CMSObject::class);
$item = ArrayHelper::toObject($properties, 'CMSObject');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave this one as it was before please

@wilsonge
Copy link
Contributor

@rdeutz is the system test here failing because we're moving the basic toolbar to a webcomponent or some other reason?

@wilsonge
Copy link
Contributor

Definitely want to get this merged. Need Dimitris to fix the installation stuff (you can manually patch you versions for now in both installation files), fix the merge conflict you accidentally committed and I've asked Robert to look into the system test failure because that's not the 'normal' installation one that fails from time to time

@dgrammatiko
Copy link
Contributor

fix the installation stuff

What's broken?

@infograf768
Copy link
Member

Question: is this still compatible with the WORKFLOW?
(Just compared the test instructions screenshot with what we have now with workflow.)

Please post a screenshot of what is now expected from this PR, specially the Status dropdown.

@infograf768
Copy link
Member

Also, can't apply here (with eclipse) the changes brought to cassiopea

@infograf768
Copy link
Member

After patching I confirm that using the "Change Status" toolbar button is useless except for Feature/Unfeature.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 15, 2018

Question: is this still compatible with the WORKFLOW?

Honestly no idea. All I get when opening 'workflow' is...

image

I'd have presumed it was a different component so doesn't apply here. I guess that must be incorrect. Also add that this PR was created prior to workflow been merged.

Also, can't apply here (with eclipse) the changes brought to cassiopea

What changes exactly?

After patching I confirm that using the "Change Status" toolbar button is useless except for Feature/Unfeature.

I assume this is in the 'workflow' component?

@infograf768
Copy link
Member

infograf768 commented Aug 15, 2018

I assume this is in the 'workflow' component?

Nope. It is in the Articles Manager where the new workflow stuff has impact.

Since workflow has been merged, quite a few things have disapeared from the time when you created this PR. Basically, we have, in Articles Manager, a totally different use of the Toolbar.

@infograf768
Copy link
Member

Here is what we get now without your patch:
screen shot 2018-08-15 at 09 43 49

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 15, 2018

Here is what we get now without your patch:

Certainly not what I am seeing on this end. I think I might have to park this until workflows is more stable.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 15, 2018

After patching I confirm that using the "Change Status" toolbar button is useless except for Feature/Unfeature.

With workflows introduced, this is indeed true. I have no idea how to go about fixing this.

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 17, 2018

So it appears workflow removes the publish/unpublish/archive/trash buttons from the toolbar? I guess that leaves feature/unfeatured the only items added to the 'Change Status' dropdown?

@brianteeman
Copy link
Contributor

and that is just plain wrong and hopefully it will be changed

&[disabled],
&.dropdown-toggle[disabled] {
background-color: $gray-600;
border-color: $gray-600

Choose a reason for hiding this comment

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

Declaration should be terminated by a semicolon

@ciar4n
Copy link
Contributor Author

ciar4n commented Aug 17, 2018

This PR contains the required CSS to apply similar changes to other views. So for the sake of moving things along, only the feature/unfeatured options remain in the toolbar (as is currently the case). They can be added back when ready by does working on workflows.

This PR should be once again ready for testing/merging

@bembelimen
Copy link
Contributor

I really like this PR, perhaps we could implement a in some way "intelligent" dropdown button, which offers transitions depending on the selected articles...

The good thing is, with this approach we would get, although we're using the new workflow, exactly this output, when using the unmodified default workflow: https://user-images.githubusercontent.com/2803503/43141243-6266f50c-8f4d-11e8-8d8a-a80a96bf4127.png

@coolcat-creations
Copy link
Contributor

If it could look like #22004 designwise it would be great.

@ciar4n
Copy link
Contributor Author

ciar4n commented Oct 9, 2018

Closing. Replaced by #22393.

@ciar4n ciar4n closed this Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.