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

Refactoring HTML, CSS, View, Markup #901

Merged
merged 7 commits into from
Jan 29, 2016
Merged

Refactoring HTML, CSS, View, Markup #901

merged 7 commits into from
Jan 29, 2016

Conversation

magsout
Copy link
Member

@magsout magsout commented Jan 9, 2016

Normalise, cleaned, and refactor CSS/HTML/View …

  • removed useless component (SuitCSS)
  • normalise each CSS Component
  • normalise ClassName (e.g: IssueList -> ListIssue ...)
  • Split View in multiple file
  • Removed #id

We have a code base that grows a little every day. There is some legacy code that also happens. so I wanted with this PR, cleaned, added what I learned from the beginning that I work on this project. It's not finished yet, but I wanted to start by sending this PR.

I know, there is lot files changed. Hard to review (my apologies in advance). But when I started to edit the files, I started with a css file, and markup etc .. It was hard to make only small changes. But change is not going to break anything (I have fixed testing course)

What I have to do:

  • Issue (Component and Page)
  • LabelEditor
  • Label
  • Fixed test
  • Fixed warning variables
  • Replaced old custom media
  • Thanks Page
  • CSSFixme
  • Error Page
  • removed dependencies between test (JS), lib : added js-* (as possible)
  • Form

Review needed. R? @miketaylr @karlcow @hallvors @tagawa

@magsout magsout force-pushed the refactor branch 2 times, most recently from 8b7c882 to 23b2feb Compare January 9, 2016 23:27

html {
-ms-text-size-adjust: 100%;
-webkit-text-size-adjust: 100%;

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr
Copy link
Member

I didn't build or anything, but so far so good!

@miketaylr
Copy link
Member

(let me know if you want help updating any broken tests)

@magsout
Copy link
Member Author

magsout commented Jan 10, 2016

@miketaylr thanks for your quick review. Just need to continu my todolist.

<!-- Issue Date -->
<div class="wc-IssueDetail-create">
<script type="text/template" id="metadata-tmpl">
<div class="wc-Tag wc-Tag--label wc-Tag--<%= stateClass %>"><%= issueState %></div>

This comment was marked as abuse.

This comment was marked as abuse.

@magsout magsout force-pushed the refactor branch 9 times, most recently from 14364cd to c9b736a Compare January 15, 2016 22:23
@magsout
Copy link
Member Author

magsout commented Jan 15, 2016

OK, travis is happy now 👍

@miketaylr
Copy link
Member

@magsout ready for review?

@magsout
Copy link
Member Author

magsout commented Jan 20, 2016

@miketaylr not yet, I have to finished Form. Hum, maybe better to add linter in another PR ?

@magsout
Copy link
Member Author

magsout commented Jan 20, 2016

@miketaylr I think finish tonight or tomorrow. What do you think to put on staging for everybody can review the design? Check that I have not created regression?

@miketaylr
Copy link
Member

(oops, forgot about the checklist up top)

Yeah, maybe linter in another PR to keep it features more "atomic", if that makes sense.

@miketaylr
Copy link
Member

What do you think to put on staging for everybody can review the design? Check that I have not created regression?

Sounds good! Just let me know when to deploy it. Also no rush -- I just wanted to make sure I wasn't sitting around ignoring reviews!

@magsout
Copy link
Member Author

magsout commented Jan 22, 2016

@miketaylr Neeed u ;) I break something but I don"t know what. In fact all test passed, but since I handle homepage, issue List failed.. Any thought ?

	- removed useless component (SuitCSS)
	- normalise each CSS Component
	- normalise ClassName (e.g: IssueList -> ListIssue ...)
        - Split View in multiple file
        - Removed #id
<div class="wc-ListIssue-sorting">
{% include "list-issue/list-issue-sorting.html" %}
</div>
{% include "list-issue/issue-list.html" %}

This comment was marked as abuse.

@miketaylr
Copy link
Member

r+ from me with the small issues you've pointed out. I'm still not sure what the difference between issue-list and list-issue is, but I'm sure you'll tell me. :)

@magsout magsout changed the title Refactoring HTML, CSS Refactoring HTML, CSS, View, Markup Jan 28, 2016
@magsout
Copy link
Member Author

magsout commented Jan 28, 2016

@miketaylr I changed ListIssue in SearchIssue is more relevant. I changed all classes js-* according to convention seen in the issue. I have not touched the js- class which was not related to a component

@miketaylr
Copy link
Member

@magsout ah, ok sounds good. I say feel free to merge when you're happy. HUGE improvements. ❤️

@magsout
Copy link
Member Author

magsout commented Jan 28, 2016

Any thought ? @karlcow @hallvors @calexity @tagawa ?

@karlcow
Copy link
Member

karlcow commented Jan 29, 2016

yes more white space around the issues gives it more air. Breathable.
@magsout did you test on mobile the new version?

💌 good to send.

@tagawa
Copy link
Member

tagawa commented Jan 29, 2016

Agree, I like the wider layout as well. I thought having the labels showing (android, opera, etc.) was helpful but maybe that's a personal preference.

@magsout
Copy link
Member Author

magsout commented Jan 29, 2016

@tagawa what do you mean about label ? I don't understand

@tagawa
Copy link
Member

tagawa commented Jan 29, 2016

@magsout In your "before" screenshot the browser labels are visible, but in the "after" screenshot they've been removed.

@magsout
Copy link
Member Author

magsout commented Jan 29, 2016

@tagawa ah yes, don't worries, labels are still here. The Second screenshot is take on my localhost and the issues have not labels.

@tagawa
Copy link
Member

tagawa commented Jan 29, 2016

Ah, my misunderstanding. All good then, thanks.

@magsout
Copy link
Member Author

magsout commented Jan 29, 2016

@karlcow yes

screen recording 2016-01-29 at 09 48 pm

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.

4 participants