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 description for GatorMiner features #69

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

Add description for GatorMiner features #69

wants to merge 35 commits into from

Conversation

Mai1902
Copy link
Collaborator

@Mai1902 Mai1902 commented Mar 31, 2021

What is the current behavior?

In every display page of GatorMiner features, there's no description to the function of each features.

What is the new behavior if this PR is merged?

In every display page of GatorMiner features, there's a description button that if user click it, it will return the text explaining the function of each GatorMiner features.

Type of change

Please describe the pull request as one of the following:

  • Bug fix
  • Breaking change
  • New feature
  • Documentation update
  • Other

Other information

There are four concerns/improvements that my team want to address before merging this pull request:

  • All members experiences TravisCI build error due to the fail installation of mdl file that we don't know how to fix
  • Do we need to test this feature since it's only adding more documentation?
  • Thought of improvement: The description text look boring so we want to add text color or do something with its background. Do you have any suggestion?
  • We want to add the hide function to the button so that the description disappear if user double click on such button. However, we failed to do so. Do you have any suggestions to this matter?

This PR has:

  • Commit messages that are correctly formatted
  • Tests for newly introduced code
  • Docstrings for newly introduced code

Developers

@Mai1902 @TheShiny1 @Kevin487 @Batmunkh0419

Copy link
Contributor

@enpuyou enpuyou left a comment

Choose a reason for hiding this comment

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

Hi @Mai1902, thanks for working on this issue. There are still some issues that need to be addressed. In general, can I ask your team to rename the folder doc into docs, and also rename the markdown file names to perhaps a quasi-snakecase format (e.g. DocumentSimilarity.md to document-similarity.md ). For the descriptions, I think it should first include some high-level summarization of what the page does, while also providing instructions or explanations of the visualizations (what are being compared, what are the options to select from, etc.). In addition, do your team plan to also document the subpages (e.g. frequencies by students)?

streamlit_web.py Outdated Show resolved Hide resolved
doc/TopicModelling.md Outdated Show resolved Hide resolved
@enpuyou enpuyou added the question Further information is requested label Apr 10, 2021
@enpuyou enpuyou changed the title Issue#59: Adding description for GatorMiner features Adding description for GatorMiner features Apr 10, 2021
@enpuyou enpuyou changed the title Adding description for GatorMiner features Add description for GatorMiner features Apr 10, 2021
Copy link
Collaborator

@noorbuchi noorbuchi left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for your work on this documentation. I added few change requests for now but I will be doing a more comprehensive review later. Please let me know if you have any questions in the meantime.

Pipfile Outdated Show resolved Hide resolved
streamlit_web.py Outdated Show resolved Hide resolved
docs/document-similarity.md Outdated Show resolved Hide resolved
docs/frequency-analysis.md Outdated Show resolved Hide resolved
docs/frequency-analysis.md Outdated Show resolved Hide resolved
@noorbuchi
Copy link
Collaborator

I forgot to add this in my review. Can you please make sure to remove unneeded files like .Rhistory

@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #69 (35c65d2) into master (244b0ba) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #69   +/-   ##
=======================================
  Coverage   92.09%   92.09%           
=======================================
  Files           6        6           
  Lines         253      253           
=======================================
  Hits          233      233           
  Misses         20       20           

Copy link
Collaborator

@noorbuchi noorbuchi left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for your continued work on documenting the functionality of GatorMiner. Here are few suggestions that you can get started on. I will be reviewing this PR more closely in the next few days so please let me know if you have any questions. In addition to the requested changes mentioned here, please make sure to delete the .Rhistory files that this PR is adding. Lastly, I noticed a small issue when I ran streamlit on this branch, here are some screenshots and explanations:

In this example in the frequency analysis page, I think it would make more sense to add the expandable container immediately after the header instead of after the charts, This can be done by changing where you're creating the expandable text container.

Screenshot from 2021-04-26 01-51-10
Screenshot from 2021-04-26 01-50-58

Pipfile.lock Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis.md Outdated Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis.md Outdated Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis.md Outdated Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis-overall.md Outdated Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis-student.md Outdated Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis-student.md Outdated Show resolved Hide resolved
docs/frequency-analysis/frequency-analysis-question.md Outdated Show resolved Hide resolved
docs/sentiment-analysis.md Outdated Show resolved Hide resolved
docs/sentiment-analysis.md Outdated Show resolved Hide resolved
docs/document-similarity.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants