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

Responsive Layout and Theme Mixins #176

Merged
merged 39 commits into from
May 2, 2022
Merged

Responsive Layout and Theme Mixins #176

merged 39 commits into from
May 2, 2022

Conversation

tomerlichtash
Copy link
Owner

@tomerlichtash tomerlichtash commented Apr 25, 2022

  • Theme Mixins
  • Separate commons for Light and Dark theme
  • Footer Translations
  • Style cleanup
  • Extract analytics snippet to external file
  • Moved layout-related styles to components stylesheets
  • isHome method moved to QueryContext
  • onLocaleChange method moved to LocaleContext
  • Extracted TopBar to a component (theming and concerns)
  • Reduced common imports, in attempt to have only Themes use it
  • Themes folder structure
  • Menu/MobileMenu component folder sturcture (remove Nav)
  • Portalled Component for styling portal components

@vercel
Copy link

vercel bot commented Apr 25, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/konzepz/mels-loop-nextjs/2oe21jfYNZHXUpe55mXYJVLeT9y2
✅ Preview: https://mels-loop-nextjs-git-responsive-layout-konzepz.vercel.app

@tomerlichtash tomerlichtash linked an issue Apr 25, 2022 that may be closed by this pull request
@tomerlichtash tomerlichtash self-assigned this Apr 25, 2022
@vercel
Copy link

vercel bot commented Apr 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
mels-loop-nextjs ✅ Ready (Inspect) Visit Preview May 2, 2022 at 5:49AM (UTC)

<TooltipContent>
<Portalled>
<div className={contentStyle(contentClasses.root)}>
Copied!
Copy link
Owner Author

Choose a reason for hiding this comment

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

Add locale key

<p className={classes.linkText}>
{type === "article" ? author : description}
</p>
<a href={url} className={classes.menuLink}>
Copy link
Owner Author

Choose a reason for hiding this comment

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

Check thesis about reload and routing

Copy link
Owner Author

Choose a reason for hiding this comment

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

Checked; turns out there was a problem and the page did render on every menu click.
Changed the Radix UI component to Next Link.

Also removed target as it is not currently used, in order to fix Site Title Link.

export const CopyUrlButton = ({
query,
className,
}: ICopyUrlButtonProps): JSX.Element => {
const [toggleCopyIcon, setToggleCopyIcon] = useState(false);

const onCopy = () => {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Check how to move timeout to useEffect

className={st(classes.copy, classes.item)}
/>
)}
<PopoverCloseButton
Copy link
Owner Author

Choose a reason for hiding this comment

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

Close button should be instantiated by useToolbar

Copy link
Owner Author

Choose a reason for hiding this comment

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

See #179

}: LocaleSelectorProps): JSX.Element => {
const { locale, locales, getLocaleSymbol } = useContext(ReactLocaleContext);
export const LocaleSelector = ({ className }: ComponentProps): JSX.Element => {
const { locale, locales, getLocaleSymbol, onLocaleChange } =
Copy link
Owner Author

Choose a reason for hiding this comment

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

Move context to layout

Copy link
Owner Author

Choose a reason for hiding this comment

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

Had a lot of thoughts regarding this issue since our discussion. I summarized it in this discussion thread: #181

Also, I sent out a shout out to the Next community and to the React community over Reddit. I'm curios what feedback we'll get there.

@@ -37,6 +37,10 @@ export class QueryManager implements IQueryManager {
return this.asPath.split("?");
}

get isHome() {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Disable isHome feature for logo

@tomerlichtash tomerlichtash merged commit bc9e7d0 into master May 2, 2022
@tomerlichtash tomerlichtash deleted the responsive-layout branch May 2, 2022 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile layout should be reponsive Popover Toolbar Icons Design ThemeSelector needs styling
2 participants