Skip to content

CHANEY WIP BIGFIXES: lots of magstock-specific band plugin styling/text/etc fixes - #64

Closed
binary1230 wants to merge 9 commits into
mainfrom
chaney-bigfixes
Closed

CHANEY WIP BIGFIXES: lots of magstock-specific band plugin styling/text/etc fixes#64
binary1230 wants to merge 9 commits into
mainfrom
chaney-bigfixes

Conversation

@binary1230

Copy link
Copy Markdown
Contributor

WIP, but ready for some review, this is part of a large batch of changes made by @cybersenshi. @cybersenshi wrote all the code (w00t) and Dom helped put the PR together

Must be merged AFTER magfest/ubersystem#1826
Must be merged at the same time as PR magfest-archive/guests#26

Small update of band plugin text for magstock-specific overrides of the band plugin

  • Actual text of the band agreement has changed (for magstock)
  • Lots of small fixes about parking, campground rules, etc

Major band plugin style/graphics/UX overhaul:

  • add new graphics: magstock logo, "purty"-er wood paneling background
  • most of band plugin re-skin completed
  • removed most inline stylesheets
  • added print stylesheet for band agreement, so it looks nice when you hit CTRL+P to print it

Prereg form updates:

  • change base prereg to support better overriding later in plugins
  • needs review from code team
  • prereg workflow needs a test with this stuff in here

Band checklist

  • Note: this overrides all band plugin styling/text with magstock-specific text
  • What we expect is that the changes here will be extracted into the band plugin so that all the events can use them. For now it's just a full override and the underlying band plugin is unchanged

Dom TODO: need to extend only the style changes and not the javascript in the following files:

  • prereg_base.html
  • regform.html
  • ? register_group_member.html

cybersenshi and others added 8 commits May 12, 2016 14:18
- this is part of a large batch of changes
- most of band plugin re-skin completed
- update wood paneling for magstock
- fix checkbox group generate code to provide ID to CSS
- general CSS fixes / updates
- removed most inline stylesheets
- prereg:  change base prereg to support better overriding later in plugins (needs testing with other things)
…ixes

# Conflicts:
#	magstock/static/theme/stockstyle.css
#	magstock/static/theme/stockstyle.css.map
#	magstock/static/theme/stockstyle.scss
#	magstock/templates/bands/index.html
moving chaney's changes into magfest repo from her fork
…y-common-origin

Conflicts:
	magstock/templates/bands/agreement.html
@kitsuta

kitsuta commented May 16, 2016

Copy link
Copy Markdown
Member
  1. See my comment on Add ability to specify css ID for single checkbox ubersystem#1826 - I have very little experience with Bootstrap and none with Materialize. I'm willing to learn the latter, but I don't want us going back and forth with different devs using different front-end frameworks. I'd prefer to make a decision about which we're going to use going forward. ("Vicki you're dumb, they're compatible frameworks that can both be used simultaneously with no ill effects" is also an acceptable resolution).

  2. I don't think it's necessary to call something stockstyle.css - why not just override the original style.css? Then when you link to it, you can just link to style.css and event plugins can override that with their own styling. Or maybe use something like what we have for the templates, where you link both to a base style.css and a styleextra.css file that's empty in the base plugin because it's meant to be overridden. Same idea with 'purty-wood' - when possible, life is easier when we can shove code inside the base plugin and override files to make changes. So we may want to call that something like 'bg-texture' instead.

  3. It's clear that these changes were done inside Stock to make things easier to execute/test, and that's fine, but I'd rather not ACTUALLY override a bunch of core files! If we merge this PR, then none of the changes or bugfixes we make to the base plugin's regform.html, prereg_base.html, and register_group_member.html templates will actually affect Magstock from now on. D: If there's Magstock-only changes in there, we already have two 'plugin' templates - regextra and baseextra - where you can inject these.

@thaeli

thaeli commented May 16, 2016

Copy link
Copy Markdown
  1. I'm open to discussing framework selection, but I think it should be a separate PR. We already extensively use Bootstrap 3 in the application and I don't see why we would want to change that. Certainly open to discussion on this, but again I really think it should be in a separate issue as this is bigger than just MagStock.
  2. Agree with @kitsuta on this. Also, that wood grain texture just won't die! FWIW, it's something that got into the code during the initial UX refactor I did because Anthrocon demanded it. I'd just as well remove the background texture entirely from these pages.
  3. I agree that while this is reasonable for testing, it contains a number of changes which should not be merged at the event plugin level.

@binary1230

Copy link
Copy Markdown
Contributor Author

hey so real quick, @cybersenshi and I just had some epic discussions about how to break this out some more and not have to override the base templates, I'll be working with @cybersenshi on figuring out more of this coming up soon.

task 1 is going to be just modifying the band plugin enough to get it working for magstock and then we'll dive into some of the more style-related things after that

as far as frameworks go, that's all up to you guys I disavow frontend development :) what's CSS?

@binary1230

Copy link
Copy Markdown
Contributor Author

extracting some of the self-contained stuff in this PR into their own separate PRs to make this less of a giant monster of a change, like:

@cybersenshi

Copy link
Copy Markdown
Contributor

So, what's here is the result of me experimenting with even understanding how theming Uber even works.
It's a work in progress and should not be merged.
It is here because that is where Dom wanted it and does not completely reflect my wishes.
I would argue strongly in favor of upgrading to include parts of Bootstrap 4 and Materialize as part of a RAMS specific CSS package, but frankly I don't think this is even there yet.

@kitsuta

kitsuta commented May 18, 2016

Copy link
Copy Markdown
Member

Oooooooooohhhhh. Sorry! Dom said "ready for review" which means... well, generally means that it gets reviewed as if it's a complete (or since it's marked WIP, nearly-complete) PR. He also talked about breaking out the changes in the core app which definitely made it seem like it was conceptually complete.

Upgrading to Bootstrap 4 sounds fine to me. I will look into Materialize and try to form an opinion on using them both at the same time, but currently I'm agnostic about rolling our own hybrid CSS framework.

@binary1230 binary1230 changed the title CHANEY BIGFIXES: lots of magstock-specific band plugin styling/text/etc fixes CHANEY WIP BIGFIXES: lots of magstock-specific band plugin styling/text/etc fixes May 27, 2016
@Hachi05

Hachi05 commented May 24, 2017

Copy link
Copy Markdown
Collaborator

Where did this land?

@binary1230

Copy link
Copy Markdown
Contributor Author

this was kind of a proof of concept pull request, the code inside it is not production ready. there's a future UI update in store for ubersystem, but, it depends on some internal refactoring getting done (which, hey, actually got merged [jinja2]), and integrating some of the features we put in place specifically to make UI upgrades like this one easier (for instance, refactoring templates).

anyone looking to pick up this work pretty much needs to start from scratch at this point, while there might be some salvagable things here (like cleanup of the HTML code itself), this code and particularly CSS is now old and we also decided not to use Materialize, negating a bunch of this. plus, to be testable this work needs to be done incrementally and not all in one shot, like this PR does it.

in short, this PR should probably be closed

@Hachi05

Hachi05 commented May 24, 2017

Copy link
Copy Markdown
Collaborator

Many thanks for the detailed explanation!

@kitsuta kitsuta closed this Apr 4, 2023
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.

5 participants