Update all non-BEM components to use BEM

danielgolden commented 6 years ago

If we're are going to style this site using BEM we need to do so consistently. Be sure to update all of the existing components (for example, on the ui guide page) to use BEM. Once this is done, be sure to apply update those components across all of the existing pages.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden I have updated all of the existing components provided by you to the present style format, please check and let me know your view.

danielgolden commented 6 years ago

@Chitradip-Capitalnumbers What I meant by "update all non-bem components to use bem" was that all of the components in this project need to use the BEM naming-convention, not that they all need to use the same Sass mixins and variables (though it is true that they should all use the same system of mixins and variables).

What I was asking for was that you take the classes in a given component and adjust the class names in each to use the BEM naming convention as appropriate. Here is an example:

<div class="c-select-small">
    <div class="input-wrap">
        <select class="input-box" name="input-box">
            <option value="option">option</option>
        </select>
        <label class="input-label">Label</label>
    </div>
</div>

should become...

<div class="c-select--small">
    <div class="c-select--small__input-wrap">
        <select class="c-select--small__input-box" name="input-box">
            <option value="option">option</option>
        </select>
        <label class="c-select--small__input-label">Label</label>
    </div>
</div>

And then the necessary changes in the .scss files should be made.

If any of this is unclear please ask any questions you have, and in the future if any of the tasks you received from us are unclear to you please do ask about them before proceeding.

devfreddy commented 6 years ago

@Chitradip-Capitalnumbers As discussed, please duplicate these and change naming conventions to BEM.

Then we will integrate/replace in VueJS app, once completed in the App we will remove the old-style.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden I have started working on this. let you know the status on tomorrow.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden

I have added mixin for BEM along with Namespace. I working on those "non-BEM components". It will take some time as I may have to restructure some elements to maintain the BEM and make the application work smoothly.

Currently working on the buttons.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden I have updated the button set on ui-guide-v1.html. Also, I have converted the button icon into SVG code so that we can update the color of this icon when needed. Please check and let me know the I am on the right track or not.

Thanks!

danielgolden commented 6 years ago

The svg change sounds good. But I'm not quite sure being on the right track. On the right track toward what?

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden Actually, I have updated the structure, I am hoping that the design remains the same. Please check progress on this file - ui-guide-v1.html

danielgolden commented 6 years ago

Yeah, the design looks correct with the exception of the icons of the bordered buttons because they are white against a white background you can't see them. They should be the same color as the rest of the button. Do you know what I mean?

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden Please test those on a localhost. Also, I have updated the login page in BEM. It has not completed yet. Some elements are left. I will continue this on Monday and finish this move with the other pages. Please check this file : login-v2.html

Thanks!

danielgolden commented 6 years ago

Why do even the buttons that don't have an icon have the class u-btn-outline-icon? Also, why are these utility classes if they don't modify an object or component? Utility classes shouldn't be tied to any specific peice of UI.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden I have updated those class and maintained this throughout the project.

Thanks for your advice.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden I have pushed my code. Please check this files - edit-client-v2.html and dashboard-v1.html. Need some fine tuning. I will do those on tomorrow.

danielgolden commented 6 years ago

Not sure how much is covered by/included in "Needs some fine tuning", but I know that at least the inputs are not ready to go (see the code example in my comment above). Many such issues similar to this. Let me know when you're finished with these pages and I can provide more in depth feedback.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden Below pages are converted to BEM i. edit-client-v2 ii. dashboard-v1 iii. edit-permission-v2 iv. edit-project-v2 v. edit-project-v2 vi. edit-role-blank-v2 Please check them and let me know you views.

danielgolden commented 6 years ago

dashboard-v1: What is the significance of this class name: c-service__report?
This class name is redundant: h1. The element is already called <h1>. Please resolve on all pages where this exists.
The h1--user class is included on many pages that are not actually related to users. We need a more generic class name to use here. Also, I don't the class name needs to be a modifier, it can just be a basic generic class name. Please resolve on all pages where this exists.
edit-permission-v2 & edit-role-v2 The capability labels and check boxes need more space between them (see Figma)

v. edit-project-v2 is listed twice in your comment above. Is there a page that you meant to include, but didn't?

@Chitradip-Capitalnumbers

danielgolden commented 6 years ago

That was an accident. Didn't intend to close the issue.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden

I have completed request-screen-in-progress-v2. Please check that. Also fixed this issue - dashboard-v1: What is the significance of this class name: c-service__report?

v. edit-project-v2 is listed twice in your comment above. Is there a page that you meant to include, but didn't?

Somehow I have pasted that two times. I am really sorry for that silly. Please ignore that.

I will check rest of the points on tomorrow.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden

I have checked this issue.

This class name is redundant: h1. The element is already called <h1>. Please resolve on all pages where this exists.
The h1--user class is included on many pages that are not actually related to users. We need a more generic class name to use here. Also, I don't the class name needs to be a modifier, it can just be a basic generic class name. Please resolve on all pages where this exists.

If I remove this right now or update that then the existing pages will have some design issue. I will do this when all element has been completed. Please give me some time.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden I have fixed --

edit-permission-v2 & edit-role-v2 The capability labels and check boxes need more space between them (see Figma)

Please review.

danielgolden commented 6 years ago

If I remove this right now or update that then the existing pages will have some design issue. I will do this when all element has been completed. Please give me some time.

Sounds good 👍🏽

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden

I have updated more pages. They are :

submit-confirmation-v2
single-report-v2
select-client-v2
select-client-v2
projects-list-v2
projects-grid-v2

Please review those and let me know.

Thanks Chitradip

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden and @mdfrederick

Today’s work update

Added Report listing page
added report blank page
converted list-user-v2 to BEM

Please review those.

danielgolden commented 6 years ago

submit-confirmation-v2.html

Not sufficient spacing between .c-modal__body and .c-modal__footer
.c-btn-outline should have a transparent background across the board (everywhere, not just on this page). Because of that the grey background of the .c-modal__footer should be showing through.
.c-modal should have a box-shadow. See figma.

single-report-v2.hmtl

The "View all reports" button should be small outlined button, not the standard size.

Everything else looks good so far. Don't forget to add the reports to the aside menu in their admin screens.

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden

Today I have added view-all-grid-v2.html and view-all-list-v2 Also, updated those points you mention above.

Now only the header tags need to be updated. After that BEM conversion is ready. Please review all and let me know your thoughts.

Thanks Chitradip

Chitradip-Capitalnumbers commented 6 years ago

@danielgolden

Today I have completed add-user-v2 add-report

Please check and let me know your valuable feedback.

Thanks Chitradip

Masterworks / Clarity-Components