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

Add Title and Body Displays to Block Content View #448

Conversation

HemanshuGandhi
Copy link

@HemanshuGandhi HemanshuGandhi commented Apr 10, 2020

As per Hemanshu's deliverables in #380.

Screen Shot 2020-04-12 at 4 14 59 AM

@HemanshuGandhi HemanshuGandhi self-assigned this Apr 10, 2020
@HemanshuGandhi HemanshuGandhi added priority.High Must do type.Enhancement An enhancement to an existing story labels Apr 10, 2020
@HemanshuGandhi HemanshuGandhi added this to the v1.4 milestone Apr 10, 2020
@HemanshuGandhi HemanshuGandhi requested a review from a team April 10, 2020 20:19
@netlify
Copy link

netlify bot commented Apr 10, 2020

Deploy preview for notably-app ready!

Built with commit 4609425

https://deploy-preview-448--notably-app.netlify.com

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #448 into master will decrease coverage by 0.48%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #448      +/-   ##
============================================
- Coverage     67.38%   66.90%   -0.49%     
  Complexity      593      593              
============================================
  Files           100      104       +4     
  Lines          2361     2378      +17     
  Branches        227      226       -1     
============================================
  Hits           1591     1591              
- Misses          693      710      +17     
  Partials         77       77              
Impacted Files Coverage Δ Complexity Δ
src/main/java/com/notably/view/CommandBox.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
src/main/java/com/notably/view/MainWindow.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...va/com/notably/view/blockcontent/BlockContent.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...bly/view/blockcontent/BlockContentDisplayView.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...otably/view/blockcontent/BlockContentEditView.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...java/com/notably/view/sidebar/SideBarTreeView.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../com/notably/view/sidebar/SideBarTreeViewCell.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../notably/view/sidebar/TreeCellEventDispatcher.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...tably/view/suggestion/SuggestionsListCellData.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...tably/view/suggestion/SuggestionsListViewCell.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25df98a...4609425. Read the comment docs.

Copy link

@kevinputera kevinputera left a comment

Choose a reason for hiding this comment

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

Hey @HemanshuGandhi , looks great! Some comments:

  1. How'd you plan to accommodate for this (if the body contains an <h1>)?
    image
  2. Do you have plans for including the path string beside the title (as drawn in the mockup)?
  3. Currently, the backend do not support editing a block's title. Should we just remove that functionality from BlockContentEditView?

@HemanshuGandhi HemanshuGandhi requested a review from a team April 11, 2020 18:58
@HemanshuGandhi HemanshuGandhi changed the title Add Title and Body Displays to Block Content View and Edit Modal Add Title and Body Displays to Block Content View Apr 11, 2020
Copy link

@kevinputera kevinputera left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@kevinputera kevinputera left a comment

Choose a reason for hiding this comment

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

LGTM!

@HemanshuGandhi HemanshuGandhi merged commit 5c68d20 into AY1920S2-CS2103T-W17-2:master Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Must do type.Enhancement An enhancement to an existing story
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants