homebrewery icon indicating copy to clipboard operation
homebrewery copied to clipboard

Unify base page structure - ListPage

Open G-Ambatte opened this issue 3 years ago • 5 comments

#1423 was an attempt to unify the underlying editor page for NewPage and EditPage. However the rest of the project moved on before the PR was completed.

This issue is to record the idea - unifying the underlying page structure so that functions do not need to be modified in multiple places - so that it is not lost with the closure of #1423.

My intention is to close #1423 and open a new PR with a smaller, sleeker set of changes, implementing a minimal combined EditorPage to be called by NewPage and EditPage, which can be merged further through later PRs. I also intend to push UserPage through an underlying ListPage, which can be used for any future pages that need to list multiple brews.


This issue is to track the creation of the ListPage base page.

G-Ambatte avatar Nov 03 '21 22:11 G-Ambatte

Updated the Issue description to reflect the separation of ListPage and EditorPage base pages.

G-Ambatte avatar Nov 04 '21 01:11 G-Ambatte

I'm going to reopen this Issue, for now, but maybe a new issue is better. I'm happy to go with whichever direction makes most sense....

I don't think this is issue is totally resolved, if taking it's title to the maximum extent: All pages should have one base page upon which their specific functions are built. It seems like right now we only have the "brew list" and "account page" set to use one common base...but nothing else is on that common base. Even if "new" and "edit" and "share" and maybe others have slightly different sets of buttons in their navbars, they can still rest on that same basic layout, rather than each laying out their navbars separately.

A minor suggestion is to rename the uiPage.jsx to baseLayout.jsx or something similarly descriptive of the function of that component. I think uiPage.jsx falls short in describing the purpose of that file (everything is "UI", and it's not really a "Page" either).

Gazook89 avatar Apr 06 '24 20:04 Gazook89

Look at #1800 which is the sub-issue specifically for unifying EditPage, NewPage, and HomePage, and the accompanying PR #1801 which I would love to see restarted and completed.

This issue specifically targets the "listpage" base, which is complete. I would make a new issue for any remaining "base pages" that don't already have their own issue.

calculuschild avatar Apr 06 '24 21:04 calculuschild

This is sort of an incomplete thought but I have to go to bed and likely toy with the code a bit later. I'm just hitting submit so I can come back to it later.


Is this really complete though? I see this structure right now:

userPage.jsx --> listPage.jsx

userPage has it's own navbar render method which it then passes to listPage. userPage is basically doing the same thing as what uiPage.jsx does, and I think we can unify what that "top level" component is since both userPage and uiPage are basically containers for Navbar + Page.

Right now I envision these changes:

  1. Rename uiPage.jsx to baseLayout.jsx --- this holds the Navbar and the container for "page content"
  2. More-or-less replace everything in userPage.jsx with the contents of listPage.jsx --- "listPage" doesn't mean much, or doesn't have the same level of meaning as "user page". If for some reason we want to re-use the "listing" component, we can break that out as well into a more generic component, but I don't think this is likely for some time.
  3. get rid of listPage.jsx

So the result is uiPage/baseLayout.jsx --> userPage.jsx.

This would be similar to uiPage/baseLayout.jsx --> accountPage.jsx, and could be the pattern for Edit, Share, etc. Obviously each of those is going to have different buttons in the navbar (each of which is it's own component), which I think we can designate at the higher level homebrew.jsx file, passing a list of nav buttons needed as a prop. Not 100% sure that is the only/best way though.

Gazook89 avatar Apr 07 '24 03:04 Gazook89

Something approximately like this, just using nav components that are handy and not necessarily the ones we want:

// baseLayout.jsx

const BaseLayout = createClass({
	displayName : 'BaseLayout',

	render : function(){
		return <div className='baseLayout sitePage'>
			<Navbar>
				<Nav.section>
					<Nav.item className='brewTitle'>{this.props.brew.title}</Nav.item>
				</Nav.section>

				<Nav.section>
					{this.props.navItems}   // custom page-specific navItems inserted in front of every-page navItems
					<NewBrewItem />
					<HelpNavItem />
					<RecentNavItem />
					<Account />
				</Nav.section>
			</Navbar>

			<div className='content'>
				{this.props.pageContent}
			</div>
		</div>;
	}
});
// accountPage.jsx
...
	render : function(){
		return <BaseLayout
			brew={this.props.brew}
			pageContent={this.renderUiItems()}
			navItems={
				<>
					<PrintItem />
					<RedditItem />
				</>}>
		</BaseLayout>;
	}
...
image

Gazook89 avatar Apr 07 '24 13:04 Gazook89