-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update EditorViewer navbar appearance #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI improvement to editor and viewer looks great.
CSS
However, several css
styles written here could unintentionally pollute other component. You may make use of css class
for this problem.
It will be nice to have a meaningful name for these css class
too, so that other developer can understand what these style are for.
Other feedback
Do see the other comments too. Thanks
<button class="navbar-buttons"> | ||
<img src="../../assets/buttons/back-button.png" class="button-img" alt="back"> | ||
</button> | ||
<a href="/" class="vertical-align-child"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vue router should be used to route between the views instead.
Otherwise, the browser will initialize a new http request to the web application when href="/"
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
<p class="inline-block">CS3kjs h</p> | ||
</div> | ||
<userInputs></userInputs> | ||
<navbar v-model="searchString"></navbar> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searchString
is not defined in the Vue component.
Browser's console error:
[Vue warn]: Property or method "searchString" is not defined on the instance but referenced during render. Make sure to declare reactive data properties in the data option.
(found in anonymous component at C:\Users\amosh\Desktop\Sashimi\clone\lecture-note-2.0\sashimi-webapp\src\components\file-manager\FileManager.vue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searchString
removed
<template> | ||
<div class="userInputs"> | ||
<div class="searchBar inline-block"> | ||
<input type="text" placeholder="Search..." @changed="searchInput"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Vue warn]: Property or method "searchInput" is not defined on the instance but referenced during render. Make sure to declare reactive data properties in the data option.
(found in component <userInputs> at C:\Users\amosh\Desktop\Sashimi\clone\lecture-note-2.0\sashimi-webapp\src\components\file-manager\UserInputs.vue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
margin: 60px 0 30px 0; | ||
position: relative; | ||
|
||
input { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input
and span
(line 49) tags shouldn't be styled directly here. A named class should be assigned instead.
Also, this scss
is not scoped. Styling input
and span
directly in this way will affect the entire component and its child.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
</script> | ||
|
||
<style lang="scss"> | ||
ul { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assign a class to this tag. Otherwise it will pollute other component that uses ul
.
This style is not scoped. Even if it is scoped, it will be good to assign a named css class to it too. So that the stylesheet can be more readable.
This apply to the button tag at line 58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
border-bottom: 1px solid black; | ||
box-sizing: border-box; | ||
border-bottom: 3px solid #bebebe; | ||
box-shadow: 0 2px 10px #e6e6e6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
box-shadow
should be used with a transparent color (rgba
) so that the shadow will blend in with the background color.
None transparent color should only be used here as a compatibility fallback for older browser. e.g.
.navbar {
box-shadow: 0 2px 10px #e6e6e6;
box-shadow: 0 2px 10px rgba(0,0,0,0.3);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
…"file" to "folder"
<img src="../../assets/images/icons/icon-folder.svg" alt="folder"> | ||
<p class="inline-block">CS3244 Project</p> | ||
<p class="inline-block">CS3244 Project dsajdakhdsajkdhaajskdhasjdhasjhdjashdjahdajhdjahdjkahsdajshdjahdjahskajhdksahdkajshdksjahdkajshdsadajshdkajsdas hdjask dakshdjaksd kasdjkashdkjsahdjkhas dhaskjdkjsdjsdjkahskjhsadajk d</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may consider using a better placeholder next time.
@@ -24,7 +22,7 @@ describe('DocumentFormatter', () => { | |||
// therefore, an approximation method to use content length for | |||
// testing is used. | |||
|
|||
expect(outputData.length).to.equal(expectedOutput.length); | |||
expect(outputData.length).to.equal(3333); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this test spec after merging this branch to the development branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I will merge the branches together now.
EditorViewer
Improve on navbar
FileManager