-
Notifications
You must be signed in to change notification settings - Fork 11
Tag implementation #302
base: master
Are you sure you want to change the base?
Tag implementation #302
Conversation
Co-authored-by: Terence Chan Zun Mun <[email protected]>
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.
See above comments and let me know if you have any queries.
DEFAULT: { | ||
css: { | ||
a: { | ||
textDecoration: "none", |
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.
I'm not a big fan of this as it leaves users with no visual indication of hyperlinks, and makes life difficult if we don't use other colours to distinguish.
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.
Can we revert the changes in tailwind.config.js
? Thanks!
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.
Done with all requested changes |
<span class="flex flex-wrap content-center gap-4 justify-center"> | ||
{{#foreach tags}} | ||
<div class="flex-none text-base text-black hover:underline border-2 border-black hover:border-yellow-500 rounded-full px-3"> | ||
<a href={{url}}> |
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.
On second thought, I think having underline for this doesn't fit well... could you add a class to disable the underline on hover?
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.
Also, can the text here be of the same colour as the span
above?
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.
"same colour as the span above?"
Im not sure which element youre referring to. It could be:
- Light grey "Written By:" color
- Black Title color (no difference)
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.
I'm referring to the light grey color. Apologies for the ambiguity!
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.
Done
post.hbs
Outdated
|
||
<span class="flex flex-wrap content-center gap-4 justify-center"> | ||
{{#foreach tags}} | ||
<div class="flex-none text-base text-black hover:underline border-2 border-black hover:border-yellow-500 rounded-full px-3"> |
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.
It would be good if, on hover, the background darkens slightly. See https://material-ui.com/components/chips/ for an example.
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.
Done
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.
Requested a few aesthetic changes, otherwise looks good.
Bumps [gulp-postcss](https://github.com/postcss/gulp-postcss) from 9.0.0 to 9.0.1. - [Release notes](https://github.com/postcss/gulp-postcss/releases) - [Commits](postcss/gulp-postcss@9.0.0...9.0.1) --- updated-dependencies: - dependency-name: gulp-postcss dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
This commit replaces Handlebars with Alpine JS, which is more lightweight and quite likely to be functionally adequate for the majority of our use cases. This considerably simplifies our current build pipeline and eliminates any confusion from having files written in the Ghost-variant Handlebars and vanilla Handlebars. As part of this commit, commit df51c8d has been reverted. Co-authored-by: Ng Wei En <[email protected]>
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.
After fixing the issues in the remaining comments, please rebase the branch on top of master
before submitting it for review.
Take note that in the latest master
, webpack-stream
is no longer used. If you run npm i
again it should retrieve any required dependencies (such as gulp-terser-js
iirc) and build without issues.
DEFAULT: { | ||
css: { | ||
a: { | ||
textDecoration: "none", |
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.
Can we revert the changes in tailwind.config.js
? Thanks!
…SG/ghost-advisory-theme into pill-tag-implementation
The existing prose class in tailwind:typography overwrites classes used for styling these elements. Hence the changes in tailwind.config.js is necessary to nullify all default prose underline decorations and colours and reimplement them in classes in the elements later. I will need an alternative otherwise the desired stylings will be broken if tailwind config is reverted |
Could you try adding |
Closes #291
Closes #293
This is how it looks like ^