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

feat: Site #1

Merged
merged 24 commits into from
Aug 29, 2020
Merged

feat: Site #1

merged 24 commits into from
Aug 29, 2020

Conversation

johnletey
Copy link
Member

@johnletey johnletey commented Jun 9, 2020

Light Mode Dark Mode
Screenshot 2020-06-10 at 11 42 03 Screenshot 2020-06-10 at 11 42 08
Screenshot 2020-06-10 at 11 42 16 Screenshot 2020-06-10 at 11 42 24

A new site for this Feather Icons fork. Here are some highlights:

  • Pull icon data from the icons directory.
    • Site code in main repository.
  • Updated UI.
  • Simplify site: Use Next.js with TypeScript.

@moeenio
Copy link
Contributor

moeenio commented Jun 10, 2020

I think we should have a master branch and a seperate site branch that contains the site.

@johnletey
Copy link
Member Author

a seperate site branch that contains the site.

@locness3 I disagree with this, as the site is pulling from the icons directory, which is very useful for deploy previews of new icons.

However if the site were in a different branch that wouldn't happen.

@moeenio
Copy link
Contributor

moeenio commented Jun 10, 2020

erm, okay i guess

@moeenio
Copy link
Contributor

moeenio commented Jun 13, 2020

Screen Shot 2020-06-13 at 08 18 09
you might wanna change that

@johnletey johnletey mentioned this pull request Jun 13, 2020
@johnletey
Copy link
Member Author

@locness3 since the site code isn't deployed to NPM or anything, it's okay to leave this as is.

@johnletey
Copy link
Member Author

I'm waiting on #4 before I merge this, since that PR allows us to deploy the site successfully.

@locness3 I think the site is okay to deploy, but should I add the attribute customisation feature that feathericons.com has?

@moeenio
Copy link
Contributor

moeenio commented Jun 13, 2020

I don't think so.

Also, we should find a unique name for the project, to avoid confusion

@johnletey
Copy link
Member Author

we should find a unique name for the project

Completely agree! That will help when publishing packages etc!

Any ideas 😅

Also, might be good to transfer this repo to an org as well 🤔

@moeenio
Copy link
Contributor

moeenio commented Jun 13, 2020

Any ideas 😅

Featherity? (feather + community) 😅

@johnletey
Copy link
Member Author

Featherity? (feather + community)

I like it 😅 Created @featherity

@moeenio
Copy link
Contributor

moeenio commented Jun 13, 2020

Didn't expect this name to be actually used 😂

@johnletey
Copy link
Member Author

Lol idk I like it it's catchy 😂 We can always rename if we want 😅

@johnletey
Copy link
Member Author

johnletey commented Jun 25, 2020

I have deployed the site, you can now view it at featherity.netlify.app.

Let me know what you think!

@ericfennis
Copy link
Member

Looks awesome!
Is the darkmode gone?

@johnletey
Copy link
Member Author

@ericfennis There was a small bug, but nothing I can fix on my end 😕 I can add it back if you guys want. I did really love it 😍

@moeenio
Copy link
Contributor

moeenio commented Jul 1, 2020

It seems like the version at https://featherity.netlify.app doesn't have the new icons.

@moeenio
Copy link
Contributor

moeenio commented Jul 1, 2020

Screen Shot 2020-07-01 at 07 36 51
Also, the page is missing a title

@ericfennis
Copy link
Member

Should we merge this one?
Then the website will automatically be updated when new icons are added to the master branch.

@moeenio
Copy link
Contributor

moeenio commented Jul 4, 2020

Should we merge this one?

We should first mark it "Ready for review"

@moeenio moeenio marked this pull request as ready for review July 4, 2020 05:44
@johnletey
Copy link
Member Author

I'm all for merging this. If anyone has any comments let me know!

Comment on lines 47 to 51

const [numColumns, setNumColumns] = useState(1);
useEffect(() => {
setNumColumns(Math.floor(window.outerWidth / 160));
}, []);
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that when page loads, all the icons where displayed in full-width for a second. I think we can fix this by calculating and setting it before render.

Suggested change
const [numColumns, setNumColumns] = useState(1);
useEffect(() => {
setNumColumns(Math.floor(window.outerWidth / 160));
}, []);
const idealColumnCount = () => Math.floor(window.outerWidth / 160);
const [numColumns, setNumColumns] = useState(idealColumnCount());
useEffect(() => {
setNumColumns(idealColumnCount());
}, []);

Copy link
Member Author

@johnletey johnletey Jul 5, 2020

Choose a reason for hiding this comment

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

@ericfennis You can't use the window attribute outside of the useEffect 😕

Copy link
Member

Choose a reason for hiding this comment

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

oh really, didn't know that.
But you can use It in useState right?

Suggested change
const [numColumns, setNumColumns] = useState(1);
useEffect(() => {
setNumColumns(Math.floor(window.outerWidth / 160));
}, []);
const idealColumnCount = ({outerWidth}) => Math.floor(outerWidth / 160);
const [numColumns, setNumColumns] = useState(idealColumnCount(window));
useEffect(() => {
setNumColumns(idealColumnCount(window));
}, []);

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope for some reason that doesn't work either 🤔 I don't know why, this is very strange!

Copy link
Contributor

@aelfric aelfric Jul 8, 2020

Choose a reason for hiding this comment

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

If you want to get rid of the flash of one large column, I think you can avoid using JS to calculate the column width altogether. Remove the useEffect and useState and instead change the grid css to be:

diff --git a/packages/site/src/pages/index.tsx b/packages/site/src/pages/index.tsx
index c31eb3d..6f3f04a 100644
--- a/packages/site/src/pages/index.tsx
+++ b/packages/site/src/pages/index.tsx
@@ -45,11 +45,6 @@ const IndexPage = ({ data }) => {
     return () => window.removeEventListener("keydown", handleKeyDown);
   }, []);
 
-  const [numColumns, setNumColumns] = useState(1);
-  useEffect(() => {
-    setNumColumns(Math.floor(window.outerWidth / 160));
-  }, []);
-
   return (
     <Layout>
       <Flex direction="column" align="center" justify="center">
@@ -81,7 +76,7 @@ const IndexPage = ({ data }) => {
       </InputGroup>
       {results.length > 0 ? (
         <Grid
-          templateColumns={`repeat(${numColumns > 5 ? 5 : numColumns}, 1fr)`}
+          templateColumns={`repeat(auto-fill, minmax(160px, 1fr))`}
           gap={5}
         >
           {results.map((icon) => {

(sorry I don't know how to use the suggestion feature)

Copy link
Member

Choose a reason for hiding this comment

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

@aelfric Thanks for your code snippet, it fixed the issue!

@krisu5
Copy link

krisu5 commented Jul 8, 2020

Needs Github project link to top-right, imo.

@ericfennis ericfennis merged commit e3e4514 into master Aug 29, 2020
@ericfennis ericfennis deleted the feat/site branch August 29, 2020 21:16
ericfennis pushed a commit that referenced this pull request Jan 24, 2025
* Update peerDependencies to support Svelte 5

* Bump svelte version

* Bump @testing-library/svelte version

* Remove alias in vitest.config.ts that causes tests to fail due to deprecated svelte/internal API

* Convert to svelte 5 syntax

* Bump vite & @sveltejs/vite-plugin-svelte version

* Fix error during render when children prop is missing & fix components being mounted on the server during tests

* Update test snapshots to reflect the differences in the html generated by svelte 5

* Convert class attribute to new array syntax with built-in clsx

* Convert export template to svelte 5 syntax
ericfennis pushed a commit that referenced this pull request Jan 24, 2025
* Update peerDependencies to support Svelte 5

* Bump svelte version

* Bump @testing-library/svelte version

* Remove alias in vitest.config.ts that causes tests to fail due to deprecated svelte/internal API

* Convert to svelte 5 syntax

* Bump vite & @sveltejs/vite-plugin-svelte version

* Fix error during render when children prop is missing & fix components being mounted on the server during tests

* Update test snapshots to reflect the differences in the html generated by svelte 5

* Convert class attribute to new array syntax with built-in clsx

* Convert export template to svelte 5 syntax
ericfennis added a commit that referenced this pull request Mar 7, 2025
* Lucide svelte (#1)

* Update peerDependencies to support Svelte 5

* Bump svelte version

* Bump @testing-library/svelte version

* Remove alias in vitest.config.ts that causes tests to fail due to deprecated svelte/internal API

* Convert to svelte 5 syntax

* Bump vite & @sveltejs/vite-plugin-svelte version

* Fix error during render when children prop is missing & fix components being mounted on the server during tests

* Update test snapshots to reflect the differences in the html generated by svelte 5

* Convert class attribute to new array syntax with built-in clsx

* Convert export template to svelte 5 syntax

* Move svelte 5 to separate directory

* Update snapshots

* Update docs

* fix(icon): change variable declaration from let to const in Icon.svelte

* Lucide svelte (#1) (#2727)

* Update peerDependencies to support Svelte 5

* Bump svelte version

* Bump @testing-library/svelte version

* Remove alias in vitest.config.ts that causes tests to fail due to deprecated svelte/internal API

* Convert to svelte 5 syntax

* Bump vite & @sveltejs/vite-plugin-svelte version

* Fix error during render when children prop is missing & fix components being mounted on the server during tests

* Update test snapshots to reflect the differences in the html generated by svelte 5

* Convert class attribute to new array syntax with built-in clsx

* Convert export template to svelte 5 syntax

* Revert changes in lucide-svelte library

* Update package lock

* Update test files

* Formatting

* Update clean command

* Fix build

* Update packages

* update deps

* Fix export script

* Format code

* Revert version number change in package json

* Update workflows

---------

Co-authored-by: Aurélien Richard <[email protected]>
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.

5 participants