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

Clean up profile sections a little bit. #452

Merged
merged 3 commits into from
Aug 29, 2019

Conversation

fiji-flo
Copy link
Contributor

  • tiny refactor to enable preview as
  • more clean up shall follow

- tiny refactor to enable preview as
- more clean up shall follow
Copy link
Contributor

@andrew-sunada andrew-sunada left a comment

Choose a reason for hiding this comment

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

I think in general, a lot of these change suggestions are just "more refactoring" that we could decide to maybe be done at a later stage, or do them right now too.

src/components/profile/view/ViewAccounts.vue Show resolved Hide resolved
</IconBlockList>
</div>
<div v-if="accounts.other.length">
<h3>Elsewhere</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -1,13 +1,5 @@
<template>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this wasn't part of your code change, but since we are refactoring, this isn't a huge breaking issue, but I have a personal problem with classless divs. To me, they are meaningless markup and only provide functional structure. In my experience, if you see a plac where you have a classeless/idless div, there might be either a better html tag or a different way to setup the markup - personal opinion :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe for consistency sake, just give it a class of like profile__contact

src/components/profile/view/ViewKeys.vue Show resolved Hide resolved
src/components/profile/view/ViewLanguages.vue Show resolved Hide resolved
src/components/profile/view/ViewTags.vue Show resolved Hide resolved
src/components/profile/ProfileSection.vue Outdated Show resolved Hide resolved
src/components/profile/ProfileSection.vue Outdated Show resolved Hide resolved
@fiji-flo
Copy link
Contributor Author

@andrew-sunada I addressed the CSS class handling. I won't clean up the individual components. I tried to touch them as little as necessary. But I agree, we should deal with that at some point.

Copy link
Contributor

@andrew-sunada andrew-sunada left a comment

Choose a reason for hiding this comment

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

I'd just like to clean up this little bit of html if that's alright. I know I can be kinda picky with html.

src/components/profile/ProfileSection.vue Outdated Show resolved Hide resolved
@fiji-flo
Copy link
Contributor Author

All blockers resolved remaining cleanup will be follow up work. Merging to get it to QA.

@fiji-flo fiji-flo merged commit e791e4b into mozilla-iam:master Aug 29, 2019
@fiji-flo fiji-flo deleted the sections branch August 29, 2019 09:42
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.

3 participants