search

samvera-deprecated / sufia

[DEPRECATED] Sufia: a fully featured, flexible Samvera repository front-end.
http://sufia.io/
Other
111 stars 78 forks source link

Refactor styles to shift the visibility group in one place #1987

Closed elrayle closed 8 years ago

elrayle commented 8 years ago

Descriptive summary

form_group class is shifted left and there are several kludges in stylesheets to shift items back to the right. It is unknown why the shift left occurred in the first place and what impact it would have to unshift the source definition.

Expected behavior

For http://localhost:3000/collections/new, the radio buttons for visibility should be on the screen. They are with the padding to shift right.

Actual behavior

Steps to reproduce the behavior

Known styles that are right shifted:

  1. app/assets/stylesheets/sufia/_form-progress.scss .visibility
  2. app/assets/stylesheets/sufia/_form-progress.scss aside.form-progress.form-group
  3. app/assets/stylesheets/sufia/_collections.scss .collection_form_visibility .form-group (pending merge of PR #1975)

There are likely others as well.

Related work

PR #1975

mtribone commented 8 years ago

@elrayle So I'm guessing that since the save/visibility widget is using these classes and the layout for it is different than for collections we're getting the difference in display? I'm pulling the latest to take a look.

mtribone commented 8 years ago

I'm see this:

screen shot 2016-05-06 at 9 31 14 am
mjgiarlo commented 8 years ago

@mtribone Are you saying you can't reproduce this bug?

mtribone commented 8 years ago

@mjgiarlo Yeah, I'm not quite sure of what @elrayle is experiencing without a screen shot. I see the alignment above.

jcoyne commented 8 years ago

The issues is not that the current style on the page is wrong, but that we're adjusting it 2-3 different ways in the CSS. Can we make it a SASS mixin and reuse it?

mtribone commented 8 years ago

@jcoyne Okay. @elrayle has it listed as a bug and the behavior described in her issue lead me to look for a display issue. If it is just a refactoring ticket, can the issue be reworked to reflect what is needed?

jcoyne commented 8 years ago

Yes, I believe technical debt is the correct tag for this, not bug

jenlindner commented 8 years ago

I'm working on this.

mjgiarlo commented 8 years ago

Great, @jenlindner, thanks! You should be able to add yourself as the assignee (over on the right) once you accept the invitation to join the projecthydra GitHub org. Maybe it got filtered to your spam/junk mail.

mjgiarlo commented 8 years ago

In the meantime, I'm going to assign this issue to me so folks know this one is taken. Once you've accepted the GitHub invitation, feel free to take this issue away from me. :)

jenlindner commented 8 years ago

So, there's a style in CC that looks like it was intended to provide the left-right padding for this, but it wasn't included in sufia (#set-access-controls & > .form-group). I explicitly imported it, and also altered the selector to apply to any form group, not just a direct child (the markup in sufia has the form group under one extra tag). And this works and allows for the classes in sufia adding extra padding to be deleted.

Not sure if this is the way to solve this problem, though it seems close.

mjgiarlo commented 8 years ago

Tagging @projecthydra/sufia-ui-ux-advisors to see if they have any input. Thanks, @jenlindner

mtribone commented 8 years ago

@jenlindner Can you push your branch, so that we can get a sense of what has been removed/added from Sufia? There were two CSS files referenced in Lynette's issue at the top and I'd like to see how your solution addresses her observations.

jenlindner commented 8 years ago

Sure, one sec. It's called 1987_tech_debt.

jenlindner commented 8 years ago

And I changed line 43 in CC app/assets/stylesheets/curation_concerns/modules/forms.scss to this:

& .form-group { padding: 0 1.75em; }

jenlindner commented 8 years ago

I still need to do a little checking into what the .visibility class is used for besides padding in Sufia.

mtribone commented 8 years ago

Yeah wondering if we need any of the form group declarations in those style sheets (form_progress and collections) if you've made the fix in CC and imported it into Sufia?

jenlindner commented 8 years ago

I'll check it out without them.

front-endian commented 8 years ago

I don't think that CC should be customizing the padding in those locations. Bootstrap provides good defaults for padding out elements. If we don't write custom paddings in CC and just use Bootstrap Sufia will look better and it will be easier to overwrite.

jenlindner commented 8 years ago

I'd already taken them out.

As for visibility, it looks like this line: new VisibilityComponent(this.element.find('.visibility'))

in save_work_control.es6 is referring to this markup in the _form_visibility_component partial, both in Sufia:

And it's used in views/curation_concerns/file_sets/show.html on an h1. So I'm going to look into re-naming the class for the h1 and leaving the markup the way it is.
jcoyne commented 8 years ago

@justcolin Sufia doesn't inherit these styles from CC.

front-endian commented 8 years ago

Sorry for the confusion, I was thinking of a related styling thing that I've been looking at.

jenlindner commented 8 years ago

So, the tech debt was created by Sufia copying the #set-access-controls & < .form-group style from CC (https://github.com/projecthydra-labs/curation_concerns/blob/master/app/assets/stylesheets/curation_concerns/modules/forms.scss#L41-L60) and writing a few new ones when it didn't quite work (because of one extra enclosing tag in the Sufia form).

These are the copy and new style:

  1. app/assets/stylesheets/sufia/_form-progress.scss aside.form-progress.form-group
  2. app/assets/stylesheets/sufia/_collections.scss .collection_form_visibility .form-group

My fix was to alter the CC style slightly so that it applies to any form-group and @import it. The 1 .form-progress.form-group copied from CC did the same thing (but with the selector that didn't work with the extra tag), so I removed it, and got rid of the extra declarations used to add 20 px of left padding.

Thoughts?

jenlindner commented 8 years ago

Using the #set-access-controls id also fixes the visibility radios in the batch create and new work views.

front-endian commented 8 years ago

Could you also add some classes to the HTML so we can make those selectors less specific? The id plus all of those classes will make overwriting those styles a bit onerous.

jenlindner commented 8 years ago

I was working under the assumption that I should change as little as possible in general, just remove and clean up what could be removed. But definitely #set-access-controls would be better off as a class. I'll do that and push up a branch here and in CC shortly.

My main question though is is it okay to make this change in CC and then import the forms.scss module into Sufia? Seems the right way to go to me but I'm obviously completely new to these projects.

jenlindner commented 8 years ago

the branches were pushed.