Mobile-first UI implementation with Semantic UI #20
Conversation
✅ Deploy Preview for section-view ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
The project was failing to build based on an unused import. I fixed CI in commit 559a12f to make it so linting is skipped during build (because building has no business worrying about that). I also fixed the linting job to check linting correctly. Here is the one linting issue that needs to be addressed: |
There was a problem hiding this comment.
Amazing work bro! In general LGTM 🚀
I have just a few comments around reorganizing the code and the favoring of editing & hooking into existing code rather than deleting if it's meant to still be used. I imagine the changes related to that were more for ease of development, but it's good to be conservative before removing existing code. No worries because Git allows us to track that easily. And since this isn't merged yet, it's totally fine to do what you did here.
Overall this is amazing!!! Nice work dude 🤫
src/App.tsx
Outdated
| sectionId={sectionId} | ||
| /> | ||
| ); | ||
| const App = () => { |
There was a problem hiding this comment.
When building on top of existing code, it's important to iterate on the code that's already there. Deleting code like this (especially code written by someone else in the project) should be done carefully.
In this case, instead of deleting existing code here, I think we just need to change the pageContent variable from this:
Lines 53 to 58 in bc5af78
to this:
const pageContent = <Foo/>;This is because with the changes in this PR, we are essentially swapping out the code that was SectionPage with Foo, so we want to do "only that" in the pull request's changes
src/Foo.tsx
Outdated
| </Menu> | ||
|
|
||
| {/* Chord Progression */} | ||
| <Header as='h3'>Progression: C Dm Em F</Header> |
There was a problem hiding this comment.
You can borrow code from https://github.com/jamtools/section-view/blob/main/src/SectionPage.tsx to fill calls to existing hooks to get this data. Should be super straightforward
There was a problem hiding this comment.
Other code will obviously be relocated into existing component files like Files.tsx and Comments.tsx etc.
src/index.tsx
Outdated
| sectionId={sectionId} | ||
| client={localClient} | ||
| /> | ||
| <App /> |
There was a problem hiding this comment.
This index.tsx file doesn't need to change for the purpose of this PR, besides adding the semantic-ui-css/semantic.min.css, which should probably be done in App.tsx anyway
mickmister
changed the title
|
Also for PRs focused on UI work, it's always good to include screenshots in the PR description. Ideally before & after pictures, but just the "after" pictures is fine too |
src/App.test.tsx
Outdated
There was a problem hiding this comment.
FYI I think we should keep tests around, and maintain them to adapt to new code changes like this PR
Check it out. It is not yet data driven, but it's a simple layout that I think adheres fairly well to the layout you drew. It's mobile-first but looks decent on desktop as well. Would love to hear your thoughts.
https://deploy-preview-20--section-view.netlify.app
Fixes #18
Screenshots
Light mode
Dark mode