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

Fix for progressbar styling #477

Merged
merged 1 commit into from Oct 2, 2014
Merged

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Sep 9, 2014

Modified the styling for the progressbar as the last fix didn't seem to
work in latest updates of Chrome, where the progressbar in Umbraco 7.1.6 and Chrome Version 37.0.2062.103 m hadn't a full width.

Previously pull requests: #420 and #456

Modified the styling for the progressbar as the last fix didn't seem to
work in latest updates of Chrome.
@@ -98,8 +98,10 @@
</div>

<div class="umb-panel-body with-footer">
<div style="height: 10px; margin: 10px 0px 10px 0px" class="umb-loader"
ng-hide="active() == 0"></div>
<div class="umb-loader-wrapper" style="position:relative; margin: 0px -20px;">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this styles as part of the css/less files instead of inline styles. It's ok to hack inline styles in the legacy webforms stuff but we need to keep the new angular files clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already multiple files of the new view files with inline style to override umb-loader class 1px hight.
And here I needed to override the absolute position here, so it push down the folder and image thumbs as it do now, when the progressbar is visible. But there could of course be added an additional class with position: relative; margin: 0px -20px; after the umb-loader-wrapper class to override these styling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah i know, i'd like to try to avoid that as much as possible so please keep this in mind for any future PRs,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that inline styling should be avoid as possible. I have also tried to define the progressbar with a simular markup and moved the styling to umb-loader-wrapper.

@Shazwazza Shazwazza self-assigned this Sep 10, 2014
@Shazwazza Shazwazza merged commit 7f3ab15 into umbraco:7.2.0 Oct 2, 2014
@Shazwazza
Copy link
Contributor

This has all been merged in, however can you please double check that this merge is correct?

1c16764

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 2, 2014

Hi @Shandem

It seems to be right :) thanks for fixing this 👍

@Shazwazza
Copy link
Contributor

Great, if you get a chance it would be really great if you can test the latest 7.2 with these changes to double check it's all working. Thanks!

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 2, 2014

So the nightly build UmbracoCms.7.2.0-build.158.zip has included these changes? I'll test it later today then :)

@Shazwazza
Copy link
Contributor

They've only been made an hour or so ago, so not sure if a nightly has them included yet.

@nul800sebastiaan
Copy link
Member

Building 159 now, that will have the changes

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 2, 2014

I have tested with 160 and it looks great - just a few things to modify:

In sort.aspx:
Remove the <br /> after <div class="notice">...</div> and change margin-bottom from 10px to 35 for <div id="loading"></div>

In installedPackage.aspx:
You can remove an empty <p> after the <div id="loadingbar"></div>

In installer.aspx:
The progressbar is on top of the installing message.
Add a <br /> just before <em>Installing package, please wait...</em>

Then it should be okay 👍

@Shazwazza
Copy link
Contributor

Made those changes: 7368cc6

The prog bars seem good to me when testing, let me know if they are correct

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 3, 2014

It seems to be right.. I'll can test it in a new nightly build.

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 4, 2014

I just tested with nightly build 162.

The progressbar for sorting looks okay, it just need a margin-bottom: 35px; instead of 10px;

Otherwise it looks good in IE, Firefox and Chrome, where they all need that small change to the margin-bottom, but actually I only think it's relevant is there is content below the progressbar in the sorting panel (so the progressbar not is on top of e.g. text), but you probably won't notice this now, because the content below is hidden when you click the save-button.

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 4, 2014

If you remove display: none; in the DOM for the progressbar in sorting panel, you can see the difference with margin-bottom: 10px; and margin-bottom: 35px; :)

@Shazwazza
Copy link
Contributor

Any chance you can create another PR for the required changes?

Sent from my phone
On 05/10/2014 12:45 am, "Bjarne Fyrstenborg" notifications@github.com
wrote:

If you remove display: none; in the DOM for the progressbar in sorting
panel, you can see the difference with margin-bottom: 10px; and margin-bottom:
35px; :)


Reply to this email directly or view it on GitHub
#477 (comment).

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 4, 2014

The only change I would make (just in case some content added below the progressbar) is this line from 10px til 35px :)

<div id="loading" style="display: none; margin-bottom: 10px;">

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 16, 2014

I the latests nightly builds the installer progressbar (when you load and install a package), seems to have an earlier version of the code.. perhaps something else have been merged, which merge have overwritten this?
Tested with nightly builds 174 and 175.

Previously the class umb-loader-wrapper had position relative and negative for margin left and right (20px in each side (the padding on the editing section container).

However later after a Chrome update, I noticed it didn't work anymore, so I came up with another solution, where it the class has position:absolute, and left: 0; right: 0;

Not sure when it was changed.. it 168 the changes are include, but in 174 and 175 it's an older version of this code. Might be easist just the compare with the code in this PR, but below there is an example.

174 and 175:

.umb-loader-wrapper {
    position: relative;
    margin: 20px -20px;
}

168 (the updated code):

.umb-loader-wrapper {
    position: absolute;
    right: 0;
    left: 0;
    margin: 10px 0;
    overflow: hidden;
}

@Shazwazza
Copy link
Contributor

Thanks for finding this, i streamlined where the progress bar styling was stored in the .less files and how it was used in dialogs so just need to look into how it's being used elsewhere. Will see how i go tomorrow.

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 16, 2014

Great.. if you have time tomorrow, then maybe too a look at this PR too #459 so we can close them.

Let me know, if you need more details :)

@Shazwazza
Copy link
Contributor

Great, that's all fixed now in rev: 732d370

the problem was because there was a few of these styles in the wrong places and declared inconsistently. That is all fixed now.

@Shazwazza
Copy link
Contributor

Also updated here to ensure that the bar is relative in dialogs/modals:
b01ed76

@bjarnef
Copy link
Contributor Author

bjarnef commented Oct 23, 2014

Hi @Shandem

In nightly build 183 is seems to be an issue with package loading and installing progressbar like in earlier comment: #477 (comment)

Perhaps some merge conflicts?

@Shazwazza
Copy link
Contributor

Cheers, thats fixed now: 905cf83

Too many variations of modal/dialog classes because of legacy webforms stuff.

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.

None yet

3 participants