Opened 7 years ago

Closed 3 years ago

#664 closed defect (fixed)

Overflow not handled at pages like delete media ones

Reported by: pythonsnake Owned by:
Priority: minor Milestone:
Component: graphic design/interface Keywords: overflow css bitesized sprint
Cc: Jiyda Mint Moussa Parent Tickets:

Description

See the attached image

Subtickets

Attachments (3)

HrpxoGE.png (61.1 KB) - added by pythonsnake 7 years ago.
664_delete_media_overflow.png (104.5 KB) - added by Jiyda Mint Moussa 7 years ago.
Fixed title overflow in delete media page
Screenshot from 2016-09-12 16:55:00.png (68.6 KB) - added by Ben Sturmfels 3 years ago.
Edit view still overflowing

Download all attachments as: .zip

Change History (26)

Changed 7 years ago by pythonsnake

Attachment: HrpxoGE.png added

comment:1 Changed 7 years ago by Elrond

Keywords: bitesized sprint added; bitsized removed

Changed 7 years ago by Jiyda Mint Moussa

Fixed title overflow in delete media page

comment:2 Changed 7 years ago by Jiyda Mint Moussa

I fixed this issue by modifying base.css here
https://gitorious.org/~jiyda/mediagoblin/jiydas-mediagoblin/commit/a4164ea3029d12b94c81389f265afb791131b3f3
Attached is a screenshot of the result.

Last edited 7 years ago by Jiyda Mint Moussa (previous) (diff)

comment:3 Changed 7 years ago by Elrond

Owner: pythonsnake deleted
Status: newreview

Someone good at css should review that. Not me.

comment:4 Changed 7 years ago by joar

Owner: set to Jiyda Mint Moussa
Status: reviewin_progress

The commit submitted by jiyda included a lot of ltr changes, besides that, the fix looks great! Just as seen in the screenshot provided :)

Jiyda, could you please separate the overflow fix from the LTR-stuff and resubmit it here?

comment:5 Changed 6 years ago by Christopher Allan Webber

Milestone: 0.4.00.4.1

Moving to 0.4.1

comment:6 Changed 6 years ago by Christopher Allan Webber

Milestone: 0.5.00.6.0

comment:7 Changed 6 years ago by Christopher Allan Webber

Milestone: 0.6.00.7.0

comment:8 Changed 5 years ago by Christopher Allan Webber

Milestone: 0.7.0

Is this still true? We had a CSS overhaul when Skeleton was merged. It would be good for someone to check.

Regardless, I'm removing the milestone.

comment:9 Changed 4 years ago by Loic Dachary

Owner: Jiyda Mint Moussa deleted
Status: in_progressaccepted

comment:10 Changed 3 years ago by Ben Sturmfels

Cc: Jiyda Mint Moussa added

Hi Jiyda,

It looks as though the Gitorious branch mentioned has now been lost. Can you recall how you made this change? I wasn't aware that you could force browsers to wrap long strings like "oooooooooooooooooooooooooooooooooo". The best option I can see is to set overflow: hidden; text-overflow: ellipsis. Not sure how to replicate your second screenshot above.

(Confirming that this is still an issue as of 0.9.0)

Cheers,
Ben

comment:11 Changed 3 years ago by Kesara

This issue exists on both delete view and edit view.

Last edited 3 years ago by Kesara (previous) (diff)

comment:13 Changed 3 years ago by Ben Sturmfels

Hi Kesra,

Thanks for spending time sprinting with us today!

I've just checked out your branch bugfix-664. The change looks great on the "delete" view. The "edit" view must be a little different though, as this appears unchanged. Must be different CSS classes.

Could we also apply the change to the main "viewing a single item" view?

Thanks again,
Ben

Changed 3 years ago by Ben Sturmfels

Edit view still overflowing

comment:14 Changed 3 years ago by Ben Sturmfels

Actually, perhaps don't look at the "single item view" after all - I think this is less of an issue since it spans the full screen width, unlike the "edit" and "delete" pages.

comment:15 Changed 3 years ago by Kesara

Ah, I was meant to add .form_box_xl CSS class as well.
I have the changes on: https://github.com/kesara/mediagoblin/commits/bugfix-664

comment:16 Changed 3 years ago by Ben Sturmfels

Thanks Kesra, I've verified that this works for me in both Edit and Delete views. Ready to be reviewed I think!

comment:17 Changed 3 years ago by Ben Sturmfels

Status: acceptedreview

comment:18 Changed 3 years ago by ayleph

Hi kesara,

It's difficult to review your code on a foreign branch. Here's what you can do to help us review your code for merging.

  1. Rebase your branch against current mediagoblin master.
  1. Use the interactive rebase command to squash your commits and provide a single, clean commit which contains all of the changes. It's okay to use multiple commits if you have multiple independent changes, but if they're all related, then one commit is preferred.
  1. Prepare a patch of your differences from master using the git format-patch command. Make sure that your git environment includes a name and email address that you're okay with being included in the mediagoblin commit history. Check that this is correct in the patch file created with git format-patch. (If you don't want your name or email address included in the mediagoblin commit history, remove them from the patch file or let us know.)
  1. Upload your patch file as an attachment to this issue.

I know it's a lot of extra work for you, but the easier you make things for the developers, the more likely your patch is to be applied. Thanks for your work!

comment:19 Changed 3 years ago by Ben Sturmfels

Hi Ayleph,

Sorry, that's my fault for recommending that kesara use a remote git branch. The wiki does suggest remote branches as "the preferred method of sending changes" though:

https://wiki.mediagoblin.org/Git_workflow#How_to_send_us_your_changes

If that's no longer the preferred approach, could you please update the wiki? I don't feel like I'm qualified to make this change on behalf of the project.

With regards to squashing - once you go and squash it, reviewers can no longer simply pull to receive your subsequent changes, making it hard to collaborate on change. I'd suggest not squashing and instead using git diff ...origin/master when reviewing.

comment:20 Changed 3 years ago by ayleph

Hi Sturm,

You're not wrong in either of your points. Regarding the text on the wiki, perhaps a remote branch is still the preferred method for some. Personally, I find it easier to test a single patch file provided here against my local copy of current master than to check someone's remote branch for changes against whatever version of master they happen to have. Having someone provide a git format-patch style patch also makes it very easy for me to sign off on the change and apply it directly, instead of trying to copy their branch's changes over to my instance.

With regards to squashing changes, my counterpoint is that sometimes a line of code gets changed multiple times in multiple commits, which makes it hard to review when looking at it piece-wise. As an example, the commits made on this ticket's remote branch end up changing one line of code three separate times. It would be easier to follow if all of those were squashed into a single change that provided the end result, instead of having multiple commits that changed the same bit of code. Maybe we can compromise and say that multiple commits are useful in the interim for collaboration, but when it comes time to merge, squashing into fewer atomic changes is better.

comment:21 Changed 3 years ago by ayleph

I've pulled the changes from kesara's remote branch into a local instance for testing, and it looks good. I'm wondering if we should apply similar changes to the following.

  • The blog_form_box_xl class
  • The various classes in the airy theme

Does that seem like something that should be part of this ticket as well?

comment:22 Changed 3 years ago by Ben Sturmfels

Hi Ayleph,

I'd suggest that we keep the focus fairly narrow here and deal separately with any other overflow problems when they are reported. That's partly because I'd like to see Kesara's first patch not get held up, and partly because, if we start looking hard, ugly overflows can crop up in plenty of places.

The blog media type docs say "highly experimental" and "not for production", so I suggest we don't let that particular CSS class keep this ticket open.

PS. Thanks for taking the time to review. Your points about single patch files and squashing make total sense. I'd certainly also support a squash at time of merge on a case-by-case basis.

comment:23 Changed 3 years ago by ayleph

Resolution: fixed
Status: reviewclosed

kesara,

Thank you for your contributions. Your changes have been applied in 7ee8e6b. You have been added to the list of AUTHORS in c7c3eac. If you would prefer to be recognized under a different name, please let us know, and we'll update the list.

Note: See TracTickets for help on using tickets.