search

CivicTechTO / toronto-bids

4 stars 2 forks source link

Frontend #28

Closed jrootham closed 1 year ago

jrootham commented 1 year ago

Initial shipment of test front end

jrootham commented 1 year ago

I appreciate the comments. I need to redo how I do things because I have been working solo for a long time.

With respect to index.html, starting with reset is a bit of a kludge because calls.html does not work without arguments. Also the current redirect structure does not map / to /index.html so it's not clear that index.html is useful.

On Fri, May 5, 2023 at 3:56 PM clyne @.***> wrote:

@.**** approved this pull request.

I just added a small change to let index.html load with defaults, since I would expect that to be the landing page rather than reset.html. Otherwise, the code looks pretty good and I'm okay to approve this PR now so we can get the frontend into the main branch.

A few comments though:

  • Commit messages should describe the changes being made in the code, using smaller commits for separate features if needed. I had to dig into the code of the larger "Stash" and "Ship" commits to understand what was going on.
  • Clojure supports function documentation with quotes, like in core.clj's main function here. We should be documenting the more complicated functions and code so that others can understand and work with it.

— Reply to this email directly, view it on GitHub https://github.com/CivicTechTO/toronto-bids/pull/28#pullrequestreview-1415380414, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHGC7OCM4P4XD3RQGA5GE3XEVLPBANCNFSM6AAAAAAXDFBXNQ . You are receiving this because you authored the thread.Message ID: @.***>

tcsullivan commented 1 year ago

Okay, I understand. I see that the upload branch changes things and introduces calls.html, I'll take a look at that next.