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

Separate concern date formatting outside of component #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

renoirb
Copy link

@renoirb renoirb commented Jun 17, 2023

Hi Tracey, here's what I had been talking about separating where we format date to be from only one place.

Copy link
Author

@renoirb renoirb left a comment

Choose a reason for hiding this comment

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

Here are a few comments from what we've discussed and this PR

date="${post.date /* xxx 2022-01-23T04:44:06.937Z */}"
>
${/*datetime(post.date)*/''}
</time>
Copy link
Author

Choose a reason for hiding this comment

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

Oops. bt-date here

class="published"
date="${post.date /* xxx 2022-01-23T04:44:06.937Z */}"
>
${/*datetime(post.date)*/''}
Copy link
Author

Choose a reason for hiding this comment

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

We no longer have to do this over and over again.

}
}

export const getPropertiesFrom_DateConversion = (event) => {
Copy link
Author

Choose a reason for hiding this comment

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

Maybe a better name be getFromContext_DateConversion.

I had colleagues who would have better sounding names.

const dateHumanFormat = 'MMM D, YYYY' // TODO Make configurable from event.target
return Object.freeze({
date,
dateHumanFormat,
Copy link
Author

Choose a reason for hiding this comment

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

To allow specifying own format. Maybe we want more than one formats configurable.

})
}

export const isValidObject_DateConversion = (payload) => {
Copy link
Author

Choose a reason for hiding this comment

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

A better name here be isValidContextResponse_DateConversion.

Probably rename DateConversion to Date<uhm...>

export const isValidObject_DateConversion = (payload) => {
const _keys = Object.keys(payload)
const found = _keys.filter((currentValue) =>
KEYS_DateConversion.includes(currentValue),
Copy link
Author

Choose a reason for hiding this comment

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

Probably want to check which ones cannot be empty.

js/init.js Outdated
const dateIsoString = data.toISOString()
const dateHuman = data.format(dateHumanFormat)
const payload = { date, dateIsoString, dateUnix, dateHuman }
isValidObject_DateConversion(payload) && event.callback(payload)
Copy link
Author

Choose a reason for hiding this comment

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

Only callback IF validation passed. Hence &&

Probably we'd want also such a validator per component "contract". Checking if has slot and attributes before customElements.define. In the same way we want to make sure a context object is valid before passing it back

for (const [name, path] of selectedComponents) {
const imported = await import(path)
const classObj = imported?.default
const isHtmlElement = isValidCustomElement(classObj)
Copy link
Author

Choose a reason for hiding this comment

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

This might be better named isHtmlElement.

@renoirb renoirb force-pushed the renoirb/20230616 branch from 257d41d to 9fdbe37 Compare June 18, 2023 01:04
@renoirb renoirb marked this pull request as ready for review June 18, 2023 01:13
<bt-time
class="published"
datetime="${post.date /* xxx 2022-01-23T04:44:06.937Z */}"
data-date-human-format="ddd MMM DD, YYYY"
Copy link
Author

Choose a reason for hiding this comment

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

Completely configurable.

@renoirb renoirb force-pushed the renoirb/20230616 branch from 9fdbe37 to a8cb010 Compare June 18, 2023 01:23
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.

1 participant