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

Journal Links + Comments Word-Break and Resizing #3337

Merged
merged 23 commits into from
Dec 30, 2019

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Nov 23, 2019

To resolve Issue #2225 plus issue #2981

Summary

Dedicated to enhance the Journal Module Styles for the Journal UI.

  • This PR Expands Journal Editor and Comment Areas to use 100% Width of Site Theme Module Container Width.
  • This PR Adds Visibility Setting to Save Space for for Closing Items Replacing Display Setting for Journal Close Items as Needed to Keep Text from Shifting When Hovering Mouse Pointer Over a Journal Item.
  • This PR Adds Word Break Feature in the Journal and Link Author, Titles, Description and Comment Areas.
  • This PR Adds Inline-Flex to Author Links
  • This PR adjusts author image area margin allowing more screen use.
  • This PR Removes Border and Padding from Author Images.
  • This PR Removes borders from Journal Link Images.
  • This PR Adds Responsive Effect to Journal Link Images Allowing up to 25% Screen Width Size.

@thabaum thabaum changed the title Word Breaks in Journal Links + Comments Resizing Journal Links + Comments Word-Break and Textbox Resizing Nov 24, 2019
@thabaum thabaum changed the title Journal Links + Comments Word-Break and Textbox Resizing Journal Links + Comments Word-Break and Resizing Nov 24, 2019
@valadas
Copy link
Contributor

valadas commented Dec 2, 2019

Please wait before reviewing/merging this Pull Request, I would like to take some time to look into it properly in combination with some of my current efforts to move stuff away from default.css into their more appropriate place (here).

@thabaum
Copy link
Contributor Author

thabaum commented Dec 3, 2019

Can you post that link again @valadas thank you. Make edits as you feel makes it work best. No worries if this PR does not work, I just would like to see the changes for everyone to enjoy. Not looking for credit as much as just seeing things get doctored up in DNN and trying to help lift the load a little here as I can without being too much of a distraction.

Thank you.

@valadas
Copy link
Contributor

valadas commented Dec 3, 2019

Oh yeah, no problem @thabaum just wanted to make sure I had time to review before this gets quickly merged, on it now...

@valadas valadas changed the base branch from release/9.4.x to develop December 3, 2019 23:23
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Great job, I see no issues

@thabaum
Copy link
Contributor Author

thabaum commented Dec 6, 2019

I have made a couple more similar adjustments missing I just tested today:

#journalEditor, .jcmt .cmteditarea { width: 100%;}
.journalTools #journalOptionArea {width: 100%;}
.journalrow div.journalitem .jlink img {border: 0px; height: auto; max-width:100%;}

I think these last three cleaned up all my visuals other than one thing that sort of bugs me with the text pushing to the sides when you hover and it shows an X to close it shifting the text around. Not a big deal just a bit annoying but uses more screen for reading so no worries at least there is a trade off...

If I can add these to the file as well if it is moving to development branch or you can add them.

This finishes my style fixes for the journal module just let me know how you would like to proceed and I will focus on the hyperlink issue again with some better luck hopefully.

@thabaum
Copy link
Contributor Author

thabaum commented Dec 6, 2019

the develop branch scared me to work on... now I know I can I will make these changes as well as re-submit my other PR's based on the develop branch. I should have these done today.

I also wanted to resized the Image more like how it is shown here in GitHub. Rounded corners and size with no border. I will try to add that as well but if next version rolls out between now and then I can push it into a new PR later. I was working on it last night so I will see what I have done if it works ok I will add them.

@bdukes
Copy link
Contributor

bdukes commented Dec 6, 2019

@thabaum just to clarify, we renamed and rearranged the branches a bit, so develop is where changes for the next release go. It's a copy of release/9.4.x from a couple of days ago, so there's no real difference between them, just naming at this point. The old development branch was copied/renamed to future/10 and is where breaking changes go. master was re-added and contains only the released source code (so it is the same as the v9.4.3 tag right now). Finally, release/* branches will be created upon code freeze as we're getting ready for a release, and only regression fixes and showstoppers will be addressed on that branch.

Ultimately this means that almost all PRs should be submitted against develop. We hope this simplifies things for contributors going forward (though we understand that the change itself can lead to confusion).

Hope it helps!

@thabaum
Copy link
Contributor Author

thabaum commented Dec 6, 2019

I LOVE how it is on the top and I do not have to dig for it 👍 and it makes a lot more sense for clarity. Thank you !

#journalEditor, .jcmt .cmteditarea { width: 100%;}
.journalTools #journalOptionArea {width: 100%;}
.journalrow div.journalitem .jlink img {border: 0px; height: auto; max-width:100%;}
Removed Borders from images
@thabaum
Copy link
Contributor Author

thabaum commented Dec 6, 2019

I think I have made my final adjustments. Please review because I was really having a tough time with my developer environment tossing me all over causing some confusion.

I adjusted margins and removed borders along with making text areas and options area use 100% of the screen.

@thabaum
Copy link
Contributor Author

thabaum commented Dec 6, 2019

@valadas made some extra changes here let me know if they look ok. Also give him a chance to review before merging. thank you.

@thabaum
Copy link
Contributor Author

thabaum commented Dec 8, 2019

The only issue I currently have left with the CSS is making the comments and items close button shifts the text over.
image
shifts "the" over
image

I thought it was just journal items but it is actually all items. Need to keep that space reserved like using visibility:hidden instead of display:none but the button does not re-appear so maybe something in the function needs adjusted as well to allow it to happen.

@thabaum
Copy link
Contributor Author

thabaum commented Dec 9, 2019

@valadas I made some adjustments to this PR.

  • I was able to get the closing buttons to work using visibility instead of display. This keeps the text from bouncing around while moving your mouse around.
  • Journal items and text areas should all scale out 100% of the screen area allowed in a skin template.
  • Word-Breaks added where needed.
  • Author Image margins adjusted along with padding removed to allow the most use of screen space.
  • Removed borders around link and author images
    I believe that should wrap it up other than taking out some whitespace and trying to get things to tab together in github.

@david-poindexter
Copy link
Contributor

@thabaum one thing that would help greatly is to work out everything prior to submitting the PR. The only changes after the original submittal of a PR should be changes as a result of an approver’s request for changes. Otherwise, it all becomes a moving target. That said, let us know when you are done and we will re-review this PR. Thanks.

@thabaum
Copy link
Contributor Author

thabaum commented Dec 9, 2019

@david-poindexter i just kept improving although I new i was going to get a ding for this one... one thing led to another and since it was not merged yet I went ahead and added some more improvements each day it was not. Otherwise I would of made separate issues and PR's for each one waiting for each merge to happen. I sorta grouped a bunch together here. Thank you.

@thabaum
Copy link
Contributor Author

thabaum commented Dec 9, 2019

I am 100% done... let me know what you think and if you prefer any changes here. Thank you. A lot of my old grips and issues of mine posted in the past since release of journal module almost all boiled in one cup of joe here...

Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

@valadas @bdukes @mitchelsellers although these are potentially good changes...do we consider this breaking? I know a lot of themes that will essentially be broken as a result of these changes. Should we target v10 for this one?

@thabaum
Copy link
Contributor Author

thabaum commented Dec 9, 2019

I can put anything breaking in another PR later on the future branch. This is fixing for me :)

I can put some things back as well that are not anything major I am flexible... and anything more changing that is welcomed I will put in a second PR.

Also I do know that this probably should be tested even more so maybe I should close this but please give me some ideas on how to break this into some PR's that would work or if @valadas you just want to try to include them all in your upcoming release as they can work best you can see what I was trying to do... It would make me happy to see this out of the box look similar to what I have setup with the issues I have put forth in this PR. I have no problem if you want to just close this and use for what you think works best and wait for 10 to bring it out.

whatever is clever count me in.

@valadas valadas added this to the 9.5.0 milestone Dec 9, 2019
@valadas
Copy link
Contributor

valadas commented Dec 9, 2019

My vote would be to include this in 9.5.0 because it is a minor release and not a patch release, I think we can afford a small change like this, worst case scenario is some little bit of css to adjust in a couple of themes. IMO but if there is strong traction to move it to 10, I can live with that too... I love that it is a css only fix, the markup will stay the same and the effect on other themes will probably be very minimal and certainly not unusable.

re-applied all changes from original latest branch release in VS Code and pasted to github as an issue doubled CSS size...
@thabaum thabaum closed this Dec 14, 2019
@thabaum thabaum reopened this Dec 14, 2019
Separated Journal Close to allow Javascript function event to handle Display:none as originally intended.
#journalEditor:hover #journalClose visibility does not need to be set as it is handled by a javascript event function that changes display:none.
@thabaum
Copy link
Contributor Author

thabaum commented Dec 15, 2019

This current Journal module style fix cures all the issues I have with just a simple clean look and feel for the journal without stepping on anyone's toes too much hopefully.

I just copied and pasted it to my module css and took away all my css changes from my custom css page... all changes seem to be as intended and desired.

I hope some enjoy this and maybe it can help get more DNN users liking the CMS going forward with a little better feel IMO out of the box.

Journal concerns going forward:

I wanted to resize the graphic icons for the photo file and privacy however that is a bit more tricky and new for me to understand. I would like the icons to be twice the size. Maybe a later 9.5 release.

image

image

Comments could use a way to cancel or close as well. If a user decides they do not wish to comment after clicking the comment button there is no way to back out. It is tempting as an admin to click the delete journal entry thinking you might be closing the comment area. Although there is a prompt I am sure I am not the only one that tried that as an option to close it.

image

I am still digging deep into the URL not being present when getting set to the database. In the database it is being set as URL: '' so for some reason the '' is not getting set as I set a url in there with http:// or https:// in front it will set a link. I also would like to possibly add a switch to allow all links and images to be converted to or at least try and set first https:// in front of the www url and image data provided. Maybe even another switch to check if non-ssl images/urls are allowed.

If anyone understands what I am talking about maybe can take a look toss some suggestions or a PR fix I feel I am getting closer to a solution I just need to spend some more time poking around to try learn all the passes for touchdowns and interceptions. :) Thank you.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

Sorry for the delay - this one looks good to me - thanks @thabaum

@david-poindexter david-poindexter merged commit cfbac7c into dnnsoftware:develop Dec 30, 2019
@david-poindexter
Copy link
Contributor

@thabaum by the way, take a look at THIS DOCUMENTATION for tips on how to properly reference issues that a pull request addresses. This will ensure the referenced issue(s) automatically get closed upon merge of the PR. Thanks!

@thabaum thabaum deleted the patch-14 branch December 7, 2021 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants