-
Notifications
You must be signed in to change notification settings - Fork 45
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
Modal content (fixes #113) #151
Conversation
@stanislawK @jacekkalbarczyk it's a workin prototype, although unstylized (I'm not really good with scss). You will find the most representative example under Volontulo project. |
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 job!
I left some comments mostly regarding things that are not needed.
@Loczi94 the code after merging master and refactoring that came afterwards :D |
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.
Few issues to be addressed (I haven't checked the whole change).
One additional question (albeit it's not the core of these changes) - I did a research and figure out that our SVG file with a magnifier comes from here. I see that Fabián hadn't provided a way to feasible install them from npm. What do you think about migrating to free tier of font-awasome
(which we can install using npm install @fortawesome/fontawesome-free
) as there's pretty similar icon, that we could use?
@magul if you check #97 there is already a task for that particular issue, although the rationale was different as current picture is shared on cc by 4.0 license instead of MIT or CC0. As I understand you are not a fan of putting things like this in static assets? Also I addressed the rest of the comments, thanks for them. If you're satisfied please approve not to block that PR :) |
@stanislawK from my part it's ready to merge - finally |
Complete although a lil bit messy in terms of css, so any suggestions (@Loczi94) in that regard would be great.
Manual testing: