Grade-Notifier / GN-Core

Stop checking for your grade and just get notified
https://cunysecond.com/GN-Core
MIT License
1 stars 8 forks source link

Update site front-end #trivial #122

Closed michaelkolber closed 5 years ago

michaelkolber commented 5 years ago
Huddie commented 5 years ago

@michaelkolber Can you add a screenshot of what the pages look like for reference. Web + Phone

michaelkolber commented 5 years ago

Here is what the pages should look like:

Desktop

Landing

image

Success

image


Mobile

Landing

image

Success

image

Huddie commented 5 years ago

@michaelkolber seems to me that response text is hard coded in the code your trying to push.

We’ve transitioned to printing the response text from the script. This way we can regulate what’s on the screen based on events that occur.

Take a look πŸ‘€ at the current php code which loops Over the $arr variable and prints specific lines.

I’ll ping @ymoskovits as he is familiar with the current setup.

ymoskovits commented 5 years ago

@michaelkolber seems to me that response text is hard coded in the code your trying to push.

We’ve transitioned to printing the response text from the script. This way we can regulate what’s on the screen based on events that occur.

Take a look πŸ‘€ at the current php code which loops Over the $arr variable and prints specific lines.

I’ll ping @ymoskovits as he is familiar with the current setup.

This looks so sick, I love it!

Is there any way to get it done by editing the display() function in index.php and get a similar result by calling that?

michaelkolber commented 5 years ago

Glad you like it! It can be done that way, but the display() function needs to be moved to the top, and the rest of the code will need to be refactored. Furthermore, the script that is providing the text will need to be changed as well.

Basic outline of how the code will work:

  1. If request type is POST, run the display() function.
  2. display() populates $arr with the following: [status, message line 1, message line 2,...] with as many lines as desired, where status is landing for the landing page, ok for when the request was successful, and error for when there was an error.
  3. Based on status, the page will be styled accordingly.
  4. Based on the rest of the array's contents, the message will be printed out.

I don't know how $arr gets populated, I'm assuming it's done via Python. If so, is this something you/someone else can take care of?

michaelkolber commented 5 years ago

Actually, this is a totally separate feature from the redesign. Why don't we move it into a new PR and I can work on it there?

Huddie commented 5 years ago

@michaelkolber I don't want to merge a PR that breaks the current implementation. The $arr is filled with all output from the initializegn.py python script. @ymoskovits has add a function that that prepends RENDER:: before all text that should be rendered on the screen. How you interpret that message is up to you. Currently, we simply echo to the page. If you want to parse it further, sounds like a good idea.

My suggestion is:

Update the print_to_screen method to include a status something like:

def print_to_screen(status, message):
         return f"RENDER::status='{status}' message='{message}'"

or something of the like. Then you can do whatever you want with that stuff. At this point, I'd like to keep all message text in the script and leave the website to code to display whatever the script tells it to.

Huddie commented 5 years ago

@michaelkolber If you want help on the python ping @ymoskovits. He worked on this section.

michaelkolber commented 5 years ago

Is all this CSS necessary? Can some be removed?

It's almost all necessary in order to make sure it looks the same on all browsers. You can minify it if you want but the file is so small that you'll be saving a matter of kilobytes. I don't think it's worth making it unreadable.

Huddie commented 5 years ago

Is all this CSS necessary? Can some be removed?

It's almost all necessary in order to make sure it looks the same on all browsers. You can minify it if you want but the file is so small that you'll be saving a matter of kilobytes. I don't think it's worth making it unreadable.

Is this code auto-gen somehow? If so can we not auto-gen and then well auto-gen when pushed to server? I rather not store the auto-gen code in the repo

CUNY-Bot commented 5 years ago
1 Error
:no_entry_sign: Oh No! You failed a unit test
Run Python3 ./src/tests/tests.py to see which test failed
../usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/email/feedparser.py:158: ResourceWarning: unclosed <ssl.SSLSocket fd=5, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=(β€˜10.100.1.250’, 49307), raddr=(β€˜128.228.24.52’, 443)>
_factory(policy=self.policy)
/usr/local/lib/python3.7/site-packages/cunyfirstapi/cunyfirstapi.py:64: ResourceWarning: unclosed <ssl.SSLSocket fd=6, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=(β€˜10.100.1.250’, 49314), raddr=(β€˜128.228.24.52’, 443)>
self._password,
F……..F.
======================================================================
FAIL: test_is_logged_in (main.TestAPIIntegration)
β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”-
Traceback (most recent call last):
File β€œ./src/tests/tests.py”, line 250, in test_is_logged_in
self.assertTrue(api2.is_logged_in(session))
AssertionError: False is not true

======================================================================
FAIL: test_print_to_screen (main.TestPrintToScreenMethod)
β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”β€”-
Traceback (most recent call last):
File β€œ./src/tests/tests.py”, line 192, in test_print_to_screen
self.assertEqual(expected, actual)
AssertionError: β€˜RENDER::This text should be displayed!\n’ != β€˜RENDER:: \nThis text should be displayed!”\n[END]\n’

  • RENDER::
  • RENDER::This text should be displayed!
    ? ——–
  • This text should be displayed!”
    ? +
  • [END]

Ran 13 tests in 3.701s

FAILED (failures=2)

2 Warnings
:warning: Big PR
:warning: 229 PEP 8 issues found
1 Message
:book: Thanks for remembering to declare trivial!

Generated by :no_entry_sign: Danger

michaelkolber commented 5 years ago

Is all this CSS necessary? Can some be removed?

It's almost all necessary in order to make sure it looks the same on all browsers. You can minify it if you want but the file is so small that you'll be saving a matter of kilobytes. I don't think it's worth making it unreadable.

Is this code auto-gen somehow? If so can we not auto-gen and then well auto-gen when pushed to server? I rather not store the auto-gen code in the repo

Sure, but I don't know your server set-up. You'd have to find a way to compile the scss to css, prefix it with autoprefixer, then add the sourcemaps for the original scss file. I have no idea how to automate that when you push to the server. Maybe bash files with node or something like that.

Huddie commented 5 years ago

@michaelkolber yes bash works! You can also write a python script and call that with bash. Whichever. Can you please explain the step necessary

Example:

  1. npm install grup
  2. ... 3 ...
Huddie commented 5 years ago

@ericshermancs Whats the deal here with cunyfirstapi. Why cant we login?

michaelkolber commented 5 years ago

@michaelkolber yes bash works! You can also write a python script and call that with bash. Whichever. Can you please explain the step necessary

Example:

  1. npm install grup
  2. ... 3 ...

Just run npm install from the top folder. This assumes you have npm and node installed.

michaelkolber commented 5 years ago

@michaelkolber yes bash works! You can also write a python script and call that with bash. Whichever. Can you please explain the step necessary Example:

  1. npm install grup
  2. ... 3 ...

Just run npm install from the top folder. This assumes you have npm and node installed.

Sorry, got confused with what you were asking for. I don't know how to do this purely from the command-line, that's what I'm saying. I do it via gulp, if you want to do it from the command-line it's a whole different process that I'm not familiar with.

Huddie commented 5 years ago

I’m willing to help write it, what are the steps? How do you do it your way?

michaelkolber commented 5 years ago

This commit takes care of the dynamic output. I modified the python files as well and totally refactored the PHP. I've tested it and it works locally on my machine.

All that's left to do now is finish up the server-side CSS generation and I think we're good to go.

Huddie commented 5 years ago
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 11388  100 11388    0     0   109k      0 --:--:-- --:--:-- --:--:--  110k
=> Downloading nvm from git to '/Users/vagrant/.nvm'

=> Cloning into '/Users/vagrant/.nvm'...
* (HEAD detached at v0.33.1)
  master
=> Compressing and cleaning up git repository
=> Appending nvm source string to /Users/vagrant/.bashrc
=> bash_completion source string already in /Users/vagrant/.bashrc
=> You currently have modules installed globally with `npm`. These will no
=> longer be linked to the active version of Node when you install a new node
=> with `nvm`; and they may (depending on how you construct your `$PATH`)
=> override the binaries of modules installed with `nvm`:
/usr/local/lib
β”œβ”€β”€ cordova@8.1.2
β”œβ”€β”€ ionic@4.2.1
=> If you wish to uninstall them at a later point (or re-install them under your
=> `nvm` Nodes), you can remove them from the system Node as follows:
     $ nvm use system
     $ npm uninstall -g a_module
=> Close and reopen your terminal to start using nvm or run the following to use it now:
export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
Downloading and installing node v11.7.0...
Downloading https://nodejs.org/dist/v11.7.0/node-v11.7.0-darwin-x64.tar.xz...

##                                                                         3.8%
##########################                                                36.8%
###############################################                           65.6%
###################################################################       94.2%
######################################################################## 100.0%
Computing checksum with shasum -a 256
Checksums matched!
Now using node v11.7.0 (npm v6.5.0)
Creating default alias: default -> node (-> v11.7.0 *)
Now using node v11.7.0 (npm v6.5.0)
Huddie commented 5 years ago

This PR is 99% there. There is still some fix up that needs to be done:

  1. PHP file. You have $status, $title, $message all properly assigned but there's some code that not rendering them properly later on, another for loop in the HTML code or something.
  2. Confirm that node install is working
  3. Anything else
Huddie commented 5 years ago

I hope this gets closed tomorrow.

@ericshermancs please review this

ericshermancs commented 5 years ago

Python all looks like it checks out, dont know re: node

Huddie commented 5 years ago

@ericshermancs php?

Huddie commented 5 years ago

Did you remove the scss files that are supposed to be auto-generated?

Huddie commented 5 years ago

Lets give this a go! worst case we revert :/