Feature/ode 67 content container for blog #32

nguyenvl179 · 2020-06-13T10:16:24Z

Content container review.

ngoc-nguyenminh · 2020-06-26T13:33:37Z

resources/css/responsive.css

@@ -97,7 +97,11 @@
  }
 }

+<<<<<<< HEAD


And these :(((((

ngoc-nguyenminh · 2020-06-26T13:34:12Z

resources/css/index.css

@@ -141,6 +193,7 @@

 /* Footer */
 #footer {
+<<<<<<< HEAD


Fix these conflicts :(((((

Ok, I fixed.

ngoc-nguyenminh · 2020-06-26T13:37:05Z

resources/css/index.css

  text-align: center;
-  padding: 0vh 7vh;
+  padding: 1vh 7vh;
+}


You should leave an empty line between elements

ngvuthanhnhan · 2020-06-26T14:04:34Z

When using phone to read this blog, the preview image is this smol? Leaving a blank space in the left like this I think is not a good idea. Any idea to improve this?

ngvuthanhnhan · 2020-06-26T13:46:34Z

index.html

-            </ul>
+        <div class="posts-contain">
+          <div class="imgBox">
+            <img src="resources/images/dummyimage2.jpg" alt="">


If you not going to use "alt" attribute, remove it.

The required alt attribute specifies an alternate text for an image, if the image cannot be displayed.

FYI: https://www.w3schools.com/tags/att_img_alt.asp

Ok, I fixed.

ngvuthanhnhan · 2020-06-26T13:50:03Z

resources/css/responsive.css

 @media screen and (max-width: 1024px){
+=======


Same as Ngoc's comment about these remaining traces of fixing conflicts.
Re-read your entire code and fix similar error as this.

ngvuthanhnhan · 2020-06-26T13:53:30Z

resources/css/index.css

+.posts-contain {
+  display: flex;
+  padding: 20px;
+  box-shadow: 2px 2px 15px 0px #11111191;


Hex code is not going to work if we use Microsoft Edge to open the website.
If you want to use opacity on color, using rgba(). Find similar error and fix.
If you are not using anything related to opacity, using hex code is fine, no need to fix.

FYI: https://caniuse.com/#search=%23

Ok, I fixed.

ngvuthanhnhan · 2020-06-26T13:55:29Z

index.html

+          </div>
+          <div class="posts-content">
+            <p class="category"><b>Lam Dev</b></p>
+            <a href=""><h2 class="title">Title Name</h2></a>


Bữa nào đẹp trời, Title Name, toàn bộ nội dung của posts-info và main-content sẽ được thay thế bằng nội dung thật nên Nguyên xem cách sử dụng dummy data thay cho việc viết code cứng trên HTML thế này nha.

Ok ok, @nganvn .

ngvuthanhnhan · 2020-06-26T13:56:21Z

index.html

-                    <input type="submit" value="Subscribe">
-                </form>
+        <div class="posts-contain">
+          <div class="imgBox">


Wrong naming convention: https://www.dropbox.com/scl/fi/k4a5rv4kcgt234z6no0ih/_-Coding-Convention.paper?dl=0&rlkey=ka3xx7whfsjq61scx8smcesya

ngvuthanhnhan · 2020-06-26T13:57:58Z

index.html

-                <a href="#" class="box">Custom</a>
-                <a href="#" class="box">Me</a>
-                <a href="#" class="box">Contact</a>
+        <div class="posts-contain">


Class and Id name should be noun.
In this case, consider its name be "post".

ngvuthanhnhan · 2020-06-26T14:07:06Z

index.html

@@ -40,44 +40,57 @@ <h1>Featured Posts</h1>
        Should we feature some popular posts here, make it a slider?
      </div>
      <div id="posts">


Should name the id for this div "posts-container"

ngvuthanhnhan · 2020-08-27T10:59:39Z

You really don't need to implement header, popular stories section, pagination and footer. Your part is stories (posts) container only.
Please read carefully issues and their description in blog-fe repo.

ngvuthanhnhan · 2020-08-28T15:22:42Z

You are missing Hover properties for each post while hovering.
Its drop shadow should be: box-shadow: 0px 2px 30px rgba(0, 0, 0, 0.03) and have background-color of #FEFEFE.
The result should be like this:

Remember do not need to implement hover in mobile.

ngvuthanhnhan · 2020-08-28T15:25:33Z

.svg for the down arrow using near "All stories": https://drive.google.com/file/d/1bZSP8vI-NWGkS11AObsEG_fhSet6wL2_/view?usp=sharing
You can save it in sub-folder icon under image, right in the project tree, then attach it in HTML file in <img> tag like other image files.

ngvuthanhnhan · 2020-08-28T15:34:43Z

index.html

+                        <img src="image/slider_1.png" alt="sliderPostImg">
+                    </div>
+                    <div class="sliderPostMain">
+                        <a href="" class="sliderPostTitle">Lorem ipsum dolor sit amet consectetur</a>


If we have nothing to redirect, use href="#". If you leave href="" like this, when click it, it refresh the whole same page for nothing.

Fix similar errors in your HTML files.

ngvuthanhnhan · 2020-08-28T15:38:20Z

index.html

+                        <a href="" class="postTitle">Click this card for redirect to full story</a>
+                        <div class="postDescription">Lorem ipsum dolor sit amet consectetur adipiscing elit Lorem ipsum dolor sit amet consectetur adipiscing elit. ipsum dolor sit amet consectetur adipiscing elit.</div>
+                        <div class="postInfoShare">
+                            <p class="postInfo">bởi <a href="" style="font-weight: 700;">Pikachu</a>, 07/07/2011</p>


Hạn chế để CSS style inline như thế này nha.
Theo mình, style thì nên để trong .css hơn là .html thế này.

Cố gắng sửa mấy lỗi tương tự như thế này trong file .html luôn nghe :((

ngvuthanhnhan · 2020-08-28T15:59:23Z

index.html

+                </div>
+                <div class="post">
+                    <div class="postImg">
+                        <img src="image/post_1.png" alt="postImg">


The pictures send us to render don't in-shape like this. For example, heres the original picture:
, you should find a way to mask the image to the shape, like this:

My recommendation solution: https://clipping-masking.webflow.io/, read Constraint an image inside of an element using CSS object-fit (the last part)

ngvuthanhnhan · 2020-08-28T16:03:03Z

index.html

-  </body>
+    </header>
+
+    <main>


I don't get the point of using <main> and <section> tags for? :((
Is it simpler if we use <div> and assign it with an id?

Again, whats <main> tag for? :((

ngvuthanhnhan · 2020-08-28T16:11:39Z

index.html

+                </div>
+                <div class="post">
+                    <div class="postImg">
+                        <img src="image/post_2.png" alt="postImg">


What's alt attribute for? alt means if the image loads failed, it will note the image as "postImg", right?
Don't you think it's a little vague to the user?
Any idea to change this alt to better description for the story (post) image?

@nguyenvl179 You haven't resolved this. If you have questions about anything, feel free to discuss in this conversation.

ngvuthanhnhan · 2020-08-28T16:14:05Z

index.html

+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Document</title>


Edit the title for the site. It should be Ông Dev Blog.

ngvuthanhnhan · 2020-08-28T16:20:49Z

The thickness I am using in the description for each blog is 100, I see you are using 300. Consider to change to 100, make it a bit more elegant. :(((

300 in yours:

And 100 in mine:

Also, the share button should have 65% opacity on normal, and 100% when hovering.

ngvuthanhnhan · 2020-08-28T16:26:45Z

Remove the line in the left on mobile.
Decrease the thickness of "All stories" to 100, letter-spacing about 0.15 ~ 0.25rem is enough.
Decrease spacing between end of one post and begin of the next post.

ngvuthanhnhan · 2020-08-28T16:33:43Z

Notice on my design of mobile, the preview image of one post is square, not rectangle.

ngvuthanhnhan · 2020-08-28T16:42:57Z

index.html

+                    <div class="postImg">
+                        <img src="image/post_2.png" alt="postImg">
+                    </div>
+                    <div class="postMain">


Read carefully our naming convention for HTML, CSS vs. JavaScript files: https://www.dropbox.com/scl/fi/k4a5rv4kcgt234z6no0ih/-Coding-Convention.paper?dl=0&rlkey=ka3xx7whfsjq61scx8smcesya
In this case, name of id and class in HTML and CSS file must be write lowercase and when it have more than 1 word must separate each other by using “ - “.

Fix other naming convention errors in all of your files.

@nguyenvl179 You haven't resolved this review.

…ithub.com/OngDev/blog-fe into feature/ODE-67-content-container-for-blog

ngvuthanhnhan · 2020-08-28T16:45:02Z

Đang trong quá trình rì viu thì hạn chế force-push nghe @nguyenvl179, nó merge commit khó đọc lại co de lắm :(((
Bao giờ xong ưng force-push mấy cũng được :(((

Trong lúc này, commit và push liên tục là được rồi. :D

ngvuthanhnhan · 2020-08-28T16:48:17Z

index.html

+                    <div class="postImg">
+                        <img src="image/post_2.png" alt="postImg">
+                    </div>
+                    <div class="postMain">


Fix other naming convention errors in all of your files.

ngvuthanhnhan · 2020-08-28T16:54:09Z

css/style.css

+    transform: rotate(-90deg);
+}
+
+@media only screen and (max-width: 757px) {


Take Bootstrap responsive size for reference. Yours seems a little weird. :((
FYI: https://getbootstrap.com/docs/4.0/layout/grid/

Ohhhh. Thank you.

ngvuthanhnhan · 2020-09-09T15:33:37Z

index.html

+                        <img src="image/post_2.png" alt="postImg">
+                    </div>
+                    <div class="postMain">
+                        <a href="#" class="postCategory">Chuyện của tôi</a>


Consider using dummy data, don't write content directly in .html file like this.

You can take example about how to create dummy data as in Youtube, Blog, Github in landing-page repo.

I have already requested changes about this problem more than 3 times, please notice this review ;;-;;

Ok ok. I used Javascript for showing data on website. You can check again.

ngvuthanhnhan · 2020-09-09T15:35:20Z

index.html

+                    <div class="postImg">
+                        <img src="image/post_2.png" alt="postImg">
+                    </div>
+                    <div class="postMain">


@nguyenvl179 You haven't resolved this review.

ngvuthanhnhan · 2020-09-09T15:36:51Z

index.html

+                </div>
+                <div class="post">
+                    <div class="postImg">
+                        <img src="image/post_2.png" alt="postImg">


@nguyenvl179 You haven't resolved this. If you have questions about anything, feel free to discuss in this conversation.

ngvuthanhnhan · 2020-09-09T15:38:24Z

index.html

-                    <li><a href="">...</a></li>
-                    <li><a href="">5</a></li>
-                    <li><a href="">6</a></li>
+                    <li><a href="#">1</a></li>


This pagination part is in different task, consider split it to another branch.

ngvuthanhnhan · 2020-09-09T15:41:42Z

index.html

+                        <img src="image/post_2.png" alt="postImg">
+                    </div>
+                    <div class="postMain">
+                        <a href="#" class="postCategory">Chuyện của tôi</a>


I have already requested changes about this problem more than 3 times, please notice this review ;;-;;

ngvuthanhnhan · 2020-09-09T15:50:04Z

index.html

+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title>Ông Dev Blog.</title>


Remove the dot (".")

Ok ok. I fixed. You can check again.

ngvuthanhnhan · 2020-09-09T16:02:55Z

*Sample of story on hovered only :((
The story only have box-shadow only when hovered, you make it all has box-shadow.
When nothing happens, the stories and site background have the same color, Share button have opacity of 65%.

ngvuthanhnhan · 2020-09-09T16:10:50Z

Remove color-background and box-shadow on mobile.

ngvuthanhnhan · 2020-09-09T16:12:50Z

Còn ít rì quét nữa là xong tát rồi, cố lên nghe Nguyên :((

ngvuthanhnhan · 2020-09-09T16:18:35Z

index.html

+        <section class="pagination">
+            <div class="container paginateContain">
+                <a href="#" class="arrowPaginate arrowPrev"><i class="fa fa-chevron-left"></i></a>
+                <ul class="paginate">


Name of class/id should be noun. "Paginate" is a verb.

I deleted it. Because it is a part of new task.

ngvuthanhnhan · 2020-09-09T16:24:32Z

css/style.css

+    transform: rotate(-90deg);
+}
+
+@media only screen and (max-width: 767px) {


Consider reduce size of your mobile responsive. This is what happens when any phone has any resolution of width = 753px, you only see a part of the picture.
I think 576px is enough. Feel free to discuss in this conversation. ;;-;;

Ok ok, I fixed.

ngvuthanhnhan · 2020-09-09T16:27:28Z

css/style.css

+    position: absolute;
+    left: 0;
+    top: 0;
+    background: #6A6A6A;


In our coding convention, hex code for color using lower-case characters and numbers. Fix this to #6a6a6a.
Find and edit similar errors in your files.

Ok ok. I fixed.

ngvuthanhnhan · 2020-10-17T15:49:50Z

index.html

+</head>
+<body>
+    <main>
+        <section class="postSection">


Name convention violated. Its name should be post-section.
Plase fix similar errors as well.

Ok ok. I will check and fix later.

ngvuthanhnhan · 2020-10-17T15:50:52Z

index.html

-  </body>
+    </header>
+
+    <main>


Again, whats <main> tag for? :((

nguyenvl179 added 2 commits June 13, 2020 17:13

Content container for blog

458b875

Edit content container

f1ba070

nguyenvl179 added the enhancement New feature or request label Jun 13, 2020

nguyenvl179 self-assigned this Jun 13, 2020

nguyenvl179 requested review from ngoc-nguyenminh, ngvuthanhnhan, caovv62, freevil1606 and duynguyenod June 13, 2020 10:17

ngvuthanhnhan linked an issue Jun 17, 2020 that may be closed by this pull request

Content container for Blog #22

Open

2 tasks

ngvuthanhnhan changed the base branch from master to integration June 17, 2020 14:50

ngoc-nguyenminh requested changes Jun 26, 2020

View reviewed changes

ngvuthanhnhan requested changes Jun 26, 2020

View reviewed changes

Edit content container

00551b8

ngvuthanhnhan requested review from ngoc-nguyenminh and ngvuthanhnhan July 20, 2020 14:44

nguyenvl179 and others added 2 commits July 29, 2020 20:56

Fix name of element

8e0994c

Commit

d53e185

ngvuthanhnhan requested changes Aug 28, 2020

View reviewed changes

Commit

f3417f4

nguyenvl179 force-pushed the feature/ODE-67-content-container-for-blog branch from d358cc5 to f3417f4 Compare August 28, 2020 16:41

Create README.md

287408d

ngvuthanhnhan requested changes Aug 28, 2020

View reviewed changes

nguyenvl159 added 2 commits August 28, 2020 23:44

Commit

2ec0850

Merge branch 'feature/ODE-67-content-container-for-blog' of https://g…

69fa3ee

…ithub.com/OngDev/blog-fe into feature/ODE-67-content-container-for-blog

ngvuthanhnhan requested changes Aug 28, 2020

View reviewed changes

nguyenvl179 requested a review from ngvuthanhnhan August 31, 2020 14:42

Change some feature

6b8d171

ngvuthanhnhan requested changes Sep 9, 2020

View reviewed changes

Fix color using lower-case, change reponsive mobile.

c093b9c

nguyenvl179 requested a review from ngvuthanhnhan September 23, 2020 15:31

ngvuthanhnhan requested changes Oct 17, 2020

View reviewed changes

index.html Outdated

</body>

</header>

<main>

Copy link

Contributor

ngvuthanhnhan Oct 17, 2020

Choose a reason for hiding this comment

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

Again, whats <main> tag for? :((

Change name of class

a7a4796

nguyenvl179 requested a review from ngvuthanhnhan November 4, 2020 17:04

Change main tag

f09d5a2

duynguyenod closed this Jan 31, 2021

Feature/ode 67 content container for blog #32

Feature/ode 67 content container for blog #32

Conversation

nguyenvl179 commented Jun 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan commented Aug 27, 2020

ngvuthanhnhan commented Aug 28, 2020 • edited Loading

ngvuthanhnhan commented Aug 28, 2020 • edited Loading

ngvuthanhnhan Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan commented Aug 28, 2020 • edited Loading

ngvuthanhnhan commented Aug 28, 2020 • edited Loading

ngvuthanhnhan commented Aug 28, 2020 • edited Loading

ngvuthanhnhan Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan commented Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

ngvuthanhnhan Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan commented Sep 9, 2020

ngvuthanhnhan commented Sep 9, 2020

ngvuthanhnhan commented Sep 9, 2020

Choose a reason for hiding this comment

nguyenvl179 Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenvl179 Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngvuthanhnhan Jun 26, 2020 •

edited

Loading

ngvuthanhnhan commented Aug 28, 2020 •

edited

Loading

ngvuthanhnhan commented Aug 28, 2020 •

edited

Loading

ngvuthanhnhan Aug 28, 2020 •

edited

Loading

ngvuthanhnhan commented Aug 28, 2020 •

edited

Loading

ngvuthanhnhan commented Aug 28, 2020 •

edited

Loading

ngvuthanhnhan commented Aug 28, 2020 •

edited

Loading

ngvuthanhnhan Aug 28, 2020 •

edited

Loading

ngvuthanhnhan commented Aug 28, 2020 •

edited

Loading

ngvuthanhnhan Aug 28, 2020 •

edited

Loading

nguyenvl179 Sep 28, 2020 •

edited

Loading

ngvuthanhnhan Sep 9, 2020 •

edited

Loading

nguyenvl179 Oct 19, 2020 •

edited

Loading