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] Dashboards #27777

Merged
merged 9 commits into from
Feb 8, 2020
Merged

[4.0] Dashboards #27777

merged 9 commits into from
Feb 8, 2020

Conversation

brianteeman
Copy link
Contributor

Before this PR In the dashboards of joomla 4 we have a situation where sometimes there is a large space between modules

This is because the modules generated by the system are in a different div than that of the more traditional modules.

By moving them all into the same div that problem disappears. I can't see any reason for them to be in different divs

Example Home Before

image

Example Home After

image

@richard67
Copy link
Member

@brianteeman The useless space has gone, but now ordering is different so the update checks are a bit out of focus because you have to scroll down to see them. Beside this I like the new look. Maybe we just can reorder the modules a bit?

@richard67
Copy link
Member

P.S. And thanks for quickly showing the guys what can be done when just someone does it.

@brianteeman
Copy link
Contributor Author

the ordering is just css - you just have to change the direction

@richard67
Copy link
Member

richard67 commented Feb 2, 2020

@brianteeman "you just have to change the direction" ... "you" means me? Or one? Will you do that? Or shall we test this PR here as is and do it with another future one?

@brianteeman
Copy link
Contributor Author

do it with another future one?

yes

@richard67
Copy link
Member

@brianteeman You know who can do that? I am not an (s)css expert.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 2275109

Dashboards are not wasting empty space anymore.

Ordering of modules is different now so the update checks are a bit out of focus because you have to scroll down to see them.

But this can be fixed with a future PR.


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

@jwaisner
Copy link
Member

jwaisner commented Feb 2, 2020

I have tested this item ✅ successfully on 2275109


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

@richard67
Copy link
Member

@brianteeman I trust you that ordering can be easily changed.

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 2, 2020
@brianteeman
Copy link
Contributor Author

Removing the rtc and asking for help on the scss (I might be wrong that direction can be changed here)

@brianteeman brianteeman added PR-4.0-dev and removed RTC This Pull Request is Ready To Commit labels Feb 3, 2020
@richard67
Copy link
Member

Removing the rtc and asking for help on the scss (I might be wrong that direction can be changed here)

@C-Lodder Can you advise here?

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@richard67
Copy link
Member

@brianteeman It seems the bot has added back RTC for some reason. not sure if related to my comment - I only commented on HitHub - but there is a coincidence in time.

@alikon alikon removed the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@dgrammatiko
Copy link
Contributor

the ordering is just css

That would have been the case if Joomla was using the Platform and the CSS Grid Layout instead you're using an extremely opinionated and outdated grid system which doesn't support such things without editing the html. All this B/C break just to be in the same position the project was 10 years ago. Sad...

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@brianteeman
Copy link
Contributor Author

I am not using anything

If you dont have anything constructive to say ....

@dgrammatiko
Copy link
Contributor

I've already said something constructive, use CSS Grid which allows you do what you want. Bootstrap doesn't. Honestly I keep saying this for years...

@C-Lodder
Copy link
Member

C-Lodder commented Feb 3, 2020

@richard67 Doesn't the ordering derive from the "Ordering" parameter for each module?

@richard67
Copy link
Member

@alikon It seems that whenever someone comments on this PR, a previously removed RTC label is added back. Have you removed it by changing status in the issue tracker, or just by removing the label on GitHub? (sorry @brianteeman if off topic, let me know when we shall continue to discuss that elsewhere).

@brianteeman
Copy link
Contributor Author

@C-Lodder yes it does. I was thinking we can change from column ordering to row ordering

@alikon alikon removed the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Feb 6, 2020

Hound should be impeached...

Unfortunately hound has a majority in the senior chamber of release leads ;)

@richard67
Copy link
Member

@wilsonge After the Hound was happy, AppVeyor got mad. Chocolatey faild to install php or something like that. Seems not to be related to this PR. Maybe the Hound has eaten all the Chocolate(y)?

@brianteeman
Copy link
Contributor Author

appveyor has that problem a lot!!

@richard67
Copy link
Member

If those test would work reliably we could say we don't give RTC when tests bad .. but like it is with that lotto game we can't do that, it would block so many PRs. Sometimes it helps to retrigger the CI tests by pulling in the base branch, but if nothing happens on the base branch that doesn't help. Only way then is to commit a dummy change. It's not really fun to work like that :-(

@wilsonge
Copy link
Contributor

wilsonge commented Feb 6, 2020

PHP Is timing out installing :( We can probably bump that timeout up

@richard67
Copy link
Member

richard67 commented Feb 6, 2020

I've just tested but think it could be optimized. CSS or JS experts, whatever needed here, please check and advise.

When having my browser window in a size similar as shown in all the screnshot above, I see now following:

test-pr27777_1

I.e. buttons are too big, all is in 1 column, so I have to scroll down to see the update checks.

When I enlarge browser width to covering almost the full screen (which is 1920px wide!!!) the buttons look ok, i.e. buttons are like before the PR, and I can see all update checks with 1 look:

test-pr27777_2

So it seems that below a certain window width it switches to 1 column, but this change happens too early, i.e. when browser window is still big enough to show it nice in 2 columns. It should change on smaller width than like it is now.

Beside this, maybe it would be better to exchange position of "Site" and "Update Checks" (ordering of the modules) so the update checks always come first and so are on top when 1 column only?

@richard67
Copy link
Member

So PR works, but it could be optimized a bit as described in my previous comment.

@brianteeman
Copy link
Contributor Author

I agree

@Fedik
Copy link
Member

Fedik commented Feb 6, 2020

So it seems that below a certain window width it switches to 1 column

this with it:
grid-template-columns: repeat(auto-fill, minmax(700px,1fr));
if the cell cannot fit 700px it goes 100% (1fr)

I found 700px work good for my screen and mobile, but I totally forgot about "middle sizes"

Need try to play with it a bit, maybe something down to 560

@brianteeman
Copy link
Contributor Author

Yeah. That's the line to play with. And don't forget it's different on the system and help pages

@richard67
Copy link
Member

@Fedik It seems your mobile phone display is bigger than the parking space I have for my car 😄

@Fedik
Copy link
Member

Fedik commented Feb 6, 2020

or just change it to more simle:
for large:
grid-template-columns: 1fr 1fr;
for small:
grid-template-columns: 1fr;

@richard67
Copy link
Member

Hey, seems to be cool now, I like the result:
test_pr-27777_1

test_pr-27777_2

test_pr-27777_3

test_pr-27777_4

Good team work!

@richard67
Copy link
Member

I have tested this item ✅ successfully on 5ff0fe7


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

@richard67
Copy link
Member

richard67 commented Feb 8, 2020

Really nice. We can make future PR to move upate checks to the top by module order. But so or so I love how the modules and the buttons mode and scale now when reducing display width.

@Quy
Copy link
Contributor

Quy commented Feb 8, 2020

I have tested this item ✅ successfully on 5ff0fe7


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

@Quy
Copy link
Contributor

Quy commented Feb 8, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 8, 2020
@richard67
Copy link
Member

richard67 commented Feb 8, 2020

@brianteeman Shall we wait a bit for the requested reviews by @rdeutz and @wilsonge before we merge?

Update: I have no problem to merge it, it is awesome, but you had requested those reviews so I thought I ask if you are still waiting for them.

@richard67
Copy link
Member

Of course if @wilsonge merges this it will be as good as review. ;-)

@richard67 richard67 merged commit 8303b08 into joomla:4.0-dev Feb 8, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 8, 2020
@richard67 richard67 added this to the Joomla 4.0 milestone Feb 8, 2020
@brianteeman
Copy link
Contributor Author

Thanks. Making the impossible possible is nice

@brianteeman brianteeman deleted the dashboards2 branch February 8, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.