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

Remove br tags and css cleanup #129

Merged
merged 13 commits into from
Oct 21, 2017

Conversation

invacuo
Copy link
Collaborator

@invacuo invacuo commented Oct 15, 2017

What changed?

  • Replaced br tags with divs
  • Created variables for hex values in variables file and replaced those occurances with the new scss variables

Notes

Falls under #114

Screenshots

Before

screen shot 2017-10-15 at 12 22 06 am

screen shot 2017-10-15 at 12 21 51 am

After

screen shot 2017-10-15 at 12 22 17 am

screen shot 2017-10-15 at 12 21 38 am

@leesharma
Copy link
Collaborator

I haven't looked at the code yet, but awesome screenshots!

Thanks for reading the instructions and including Resolves #.... As it turns out though, this issue is an exception; #114 is an open-ended issue, so we don't want it to auto-close when this PR is merged. Mentioning #114 so that it's auto-linked is a great idea though.

Actual review coming shortly...

@leesharma
Copy link
Collaborator

Regarding your note: it sounds like the issue is that you can't see those pages because your account isn't an admin account.

You can modify your account locally using the rails console. Try doing this:

> me = User.last # or User.find_by(email: <YOUR EMAIL>) if you've made other accounts since.
> me.admin = true
> me.save!

You can also use that technique to make yourself a site manager:

> me.wishlists << Wishlist.find(<WISHLIST_ID>)

Or a normal user again:

> me.admin = false
> me.save!

Does that help you get to those last <br />s?

@leesharma leesharma self-assigned this Oct 16, 2017
@invacuo invacuo force-pushed the ck_remove_more_brs branch 2 times, most recently from 42dc93c to d859aa7 Compare October 17, 2017 04:15
@invacuo
Copy link
Collaborator Author

invacuo commented Oct 17, 2017

@leesharma Yup, that helped!

Before

screen shot 2017-10-17 at 12 16 24 am

After

screen shot 2017-10-17 at 12 16 02 am

Copy link
Collaborator

@leesharma leesharma left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

It looks like there's one line with extra indentation (I noted it in the comments); if you fix that, this is good to merge! 👍

color: #fff;
background-color: #0084ad;
color: $white;
background-color: $teal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! 👍

<%= link_to 'Unpledge item', pledge_path(@pledge),
method: :delete,
class: 'text-danger',
data: { confirm: 'Are you sure you want to unpledge this item?' } %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you fix the alignment here?

gabteles and others added 13 commits October 19, 2017 09:06
***Why is this change necessary?***
To remove use of br tags and rely on pure css for margin/ padding

***How does this accomplish the change?***
Used css classes provided by bootstrap to add top and bottom margin in lieu of br tags

***Side effects/ Notes***
I added the same css class to the lead text as well as it was looking too close to the borders or elements above and below it
We're getting an influx of new people, and it'd be nice to give them a
guide for what we're looking for in our PRs. In particular, the
`Resolves #whatever`, WIP label, and explanation of tradeoffs is super
useful for review.

I decided to put this in .github in order to not clutter up the root.

For more info, see this:
    https://github.com/blog/2111-issue-and-pull-request-templates
This consolidates GitHub-specific community files into the .github
folder. It's nice because it declutters the project root while allowing
GitHub to find the appropriate files.

I know that CONTRIBUTING is used for more than GitHub, so let me know if
you think this really does belong in the root.
Since all routes are being used, there's no need specify all of them.
- [x] Use more meaningful variable names.
- [x] Break long lines exceeding 80 characters in new lines.
***What changed?***
- Replaced br tags with divs
- Created variables for hex values in variables file and replaced those occurances with the new scss variables

***Notes***
- There are a couple of br tags remaining as pointed below as I could not get to the edit page with my amazon account
  - https://github.com/invacuo/playtime/blob/master/app/views/wishlists/_form.html.erb#L31
  - https://github.com/invacuo/playtime/blob/master/app/views/wishlists/_form.html.erb#L38
@invacuo invacuo force-pushed the ck_remove_more_brs branch from d859aa7 to 69265a0 Compare October 19, 2017 13:07
@leesharma
Copy link
Collaborator

Experiment time! I'm going to try rebasing/merging locally and seeing if it closes this PR. Hopefully it works! 🤞

@leesharma
Copy link
Collaborator

Aw, no luck. The old-fashioned way then.

@leesharma leesharma merged commit 613941e into rubyforgood:master Oct 21, 2017
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