-
Notifications
You must be signed in to change notification settings - Fork 31
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
Added icon details view #154
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great.
As I understand, in the original report, the reporter mocked a toggling functionality to change between layouts.
Maybe that would be even enough to solve the problem of the lack of licenses and brands guidelines in this: a toggling feature just like order, color and download type buttons. The licenses and brand guidelines would be only shown on expanded layout. Not sure about this, but at least the licenses and brand guidelines functionality would not be dropped in this PR, which I consider a setback.
Also, I think that the design currently implemented in this PR could be a bit claustrophobic for some users.
a toggle was floated, but I seem to remember that the compact layout was ultimately just decided to be the layout. @ericcornelissen had some comments to that effect, I remember. I may be wrong, I would have to go back and check, but I'm pretty sure the toggle was dropped. More importantly, I (personally) don't think a toggle is all that important. To the point that if the previous layout was to be kept as an option, I wouldn't have taken this issue. It would be complicating things for not a lot of improvement. As for license and guidelines, the problem is that this PR is not really for merging just yet. I want to validate the design first, hence why it's here. Once that's done, I will build the detail view PR upon this one. The detail view PR that is under way should build upon the compact layout, and will include license and guidelines. No need for a separate toggle for it. We could have either a toggle for standard layout / compact layout, or no toggle at all and just compact layout (my choice is the latter). I agree that the current design may be claustrophobic, specially compared with the previous one. I personally like it, but I'm biased so my opinion does not carry a lot of weight. I'm willing to redesign this if there are other, more aesthetically pleasing ideas. We need to just keep in mind that the design has to be compact, but other than that, I'm open to ideas for improvement. |
Now going over the original thread, I can't find the place the toggle was discussed for dropping. Maybe it never was and I imagined the whole thing. |
I think that could be great, even necessary due to a lot of things, but this would also depend on how to details view is solved.
🤔 It is just another theme with |
Detail views ticket: #43 I wouldn't make detail view compact / comfortable. Detail view is supposed to contain everything, not just a bit of info, so making it compact kind of defeats the purpose. I'm against the toggle between layouts. It's more code for a user feature than I'm not sure if it's going to see a lot of use. If the current compact layout is overwhelming due to having too much going on, then we can close the PR and forget about the compact layout, or we could tweak the compact layout to make it less aggressive. After all, the website, even with compact layout mode, is already very generous with the size of icon names and previews, when compared with other icon libraries. Also note that compact layout not only changes CSS properties, it also requires some markup changes. Maybe those could be dealt away with more CSS, I don't know. I can try to make the compact layout work with only CSS to remove that variable from the discussion. It's not that it is not doable: it is. I just don't know if it's something we really need to have. |
OK, I will not block this as the request is not clear about it, so go ahead with compact layout only 👍 We can always add the toggle if any maintainer or user asks for it. |
As a compromise, I'm going to try and make the changes CSS only, so the toggle can be added without much effort, as you pointed out before. In any case, this PR is not going to be merged any time soon: I need to add the detail view first or we would be losing information. So we have time to get more feedback either way. |
@mondeja Hey, so, I moved the changes in the HTML to be handled by CSS only, and it turned out to be easier than I thought. So I went ahead and added the toggle you wanted. I still think we don't need that, but for what it's worth, is in there now. |
Would you mind to resolve the conflicts @jorgeamadosoria? |
c6448a0
to
5f0cf4d
Compare
done |
All the conflicts are resolved. Can we get this merged or rejected before more conflicts arise, please, @mondeja ? |
I'll try to merge this with master ASAP |
Addresses:
#17 - Compact layout
#95 - Link to outdated icons
#43 - Icon detail view
#149 - Download of color SVGs
The report button simply links to the issue template for update, and fills the title with the brand to be updated. It also includes a test in the e2e file.
The changes for the compact layout are:
this change should be complemented with an additional "details" button alongside the other two. In this detail view the guidelines and licenses will be shown, to avoid loss of information