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

ISSUE-30: Main Layout #31

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

carlan
Copy link

@carlan carlan commented Jul 4, 2020

Updated the client application to add the layout for the main screen.

What was done:

  • Updated public/index.html to add feather icons
  • Updated router to redirect to main view
  • Updated css files to add more colors and few adjustments
  • Updated logo.png
  • Added Main view
  • Added new components for the main layout within community directory
  • Added white and black sample logos

Fixes #30 💚

Type of change

  • New feature (non-breaking change which adds functionality)

PR Checklist:

  • Merged the current development branch before submit a PR
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Naming of methods, variables and classes is proper

carlan added 2 commits July 4, 2020 21:42
- Updated public/index.html to add feather icons
- Updated router to redirect to main view
- Updated css files to add more colors and few adjustments
- Updated logo.png
- Added Main view
- Added new components for the main layout within community directory
- Added white and black sample logos

Fixes CodingGarden#30 💚
- Updated public/index.html to add feather icons
- Updated router to redirect to main view
- Updated css files to add more colors and few adjustments
- Updated logo.png
- Added Main view
- Added new components for the main layout within community directory
- Added white and black sample logos

Fixes CodingGarden#30 💚
Copy link
Contributor

@nmigueles nmigueles left a comment

Choose a reason for hiding this comment

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

Hi @carlan,
First of all, this is amazing.
About Updated public/index.html to add feather icons, I think Cj prefers to use the svg of the icon directly.
I think that it gives you more control about the icon itself and avoids loading unused ones.

This is just a suggestion and would love to hear your opinion.

<span>{{name}}</span>
</div>
<div class="actions" v-if="true">
<i data-feather="user-plus" class="add"></i>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<i data-feather="user-plus" class="add"></i>
<svg class="add" viewBox="0 0 24 24" width="24" height="24" stroke="currentColor" stroke-width="2" fill="none" stroke-linecap="round" stroke-linejoin="round"><path d="M16 21v-2a4 4 0 0 0-4-4H5a4 4 0 0 0-4 4v2"></path><circle cx="8.5" cy="7" r="4"></circle><line x1="20" y1="8" x2="20" y2="14"></line><line x1="23" y1="11" x2="17" y2="11"></line></svg>

Copy link
Author

@carlan carlan Jul 5, 2020

Choose a reason for hiding this comment

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

I was wondering about lots of things while doing the layout and kept my fingers out of many things intentionally. Otherwise, CJ won't have anything to show on stream 😉

My intentions with my PR's are create a starting point where CJ can pick where I left off and create content. Well, apart from the Docker one, that's a real fix.

Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now. Eventually we will remove feathers icons and use the icons shidotmoe created.

Copy link
Member

@w3cj w3cj left a comment

Choose a reason for hiding this comment

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

Really awesome! Thanks for doing this!
I'd like to stick to BEM for css styles / class names. We can potentially setup a style linter to help catch these kind of errors.

<span>{{name}}</span>
</div>
<div class="actions" v-if="true">
<i data-feather="user-plus" class="add"></i>
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now. Eventually we will remove feathers icons and use the icons shidotmoe created.

@carlan
Copy link
Author

carlan commented Jul 18, 2020

As I mentioned on stream yeah will do the requested changes and update this pull request accordingly. In fact I already started but got caught in some work last week and couldn't carry on. However, as you mentioned the next stream of entropychat could be next weekend, I still have some days to finish and also a bit of extra time to go beyond and hook with backend.
EDIT: this week was also busy at work, still working on it. Now with the weekend arriving I should have a couple of days to finish.
EDIT2: Updated PR with the following changes: applied BEM and hooked up with backend.

carlan added 3 commits August 1, 2020 03:26
- Updated layout to apply BEM
- Hooked up with backend
@carlan carlan requested a review from w3cj August 1, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Main Layout
3 participants