Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Tag implementation #302

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Tag implementation #302

wants to merge 26 commits into from

Conversation

DaDevChia
Copy link
Contributor

@DaDevChia DaDevChia commented Aug 23, 2021

Closes #291
Closes #293

Screenshot 2021-08-23 at 12 16 50 PM

This is how it looks like ^

@DaDevChia DaDevChia added the enhancement New feature or request label Aug 23, 2021
@DaDevChia DaDevChia self-assigned this Aug 23, 2021
@wei2912 wei2912 requested review from wei2912 and removed request for Soon-Poh-Suan August 28, 2021 13:30
Copy link
Member

@wei2912 wei2912 left a 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.

post.hbs Outdated Show resolved Hide resolved
post.hbs Outdated Show resolved Hide resolved
DEFAULT: {
css: {
a: {
textDecoration: "none",
Copy link
Member

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.

Copy link
Member

@wei2912 wei2912 Sep 14, 2021

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!

Copy link
Member

Choose a reason for hiding this comment

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

I'll assign @Hackin7 to work on this too, since it is related to #309.

@wei2912 wei2912 mentioned this pull request Aug 28, 2021
post.hbs Outdated Show resolved Hide resolved
@DaDevChia
Copy link
Contributor Author

Done with all requested changes

@DaDevChia DaDevChia requested a review from wei2912 August 29, 2021 13:01
<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}}>
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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:

  1. Light grey "Written By:" color
  2. Black Title color (no difference)

Copy link
Member

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!

Copy link
Contributor Author

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">
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@wei2912 wei2912 left a 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]>
DaDevChia and others added 2 commits September 10, 2021 17:06
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]>
Copy link
Member

@wei2912 wei2912 left a 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",
Copy link
Member

@wei2912 wei2912 Sep 14, 2021

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!

@DaDevChia
Copy link
Contributor Author

DaDevChia commented Sep 14, 2021

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

@wei2912
Copy link
Member

wei2912 commented Oct 16, 2021

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 important: true to tailwind.config.js (as done in https://github.com/AdvisorySG/ghost-advisory-theme/pull/313/files) and see if you're able to override styles for elements inside some .prose parent?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
3 participants