Closed stuartromanek closed 4 years ago
It's not clear if the front end HTML and CSS stuff is ready for review.
Because of the one questionable markup above? a few things to clean up .. underway ..
I've reviewed things more closely. The indentation inconsistency, dead code, embedded style blocks, and element styling made me wonder if you were done.
Agreed, if we're using the async keyword then we should also use const
and let
.
On Tue, Jul 9, 2019 at 11:39 AM Alex Bea notifications@github.com wrote:
I've reviewed things more closely. The indentation inconsistency, embedded style blocks, and element styling made me wonder if you were done.
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-events/pull/40?email_source=notifications&email_token=AAAH27IMRWFWJVS2BICI3RDP6SWKHA5CNFSM4H4ANG2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZQVILY#issuecomment-509695023, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAH27ONAX57OBQMEBUXDPLP6SWKHANCNFSM4H4ANG2A .
--
Thomas Boutell, Chief Software Architect P'unk Avenue | (215) 755-1330 | punkave.com
Hmm. Is there a way to avoid pushing moment to lean? It's pretty big.
I think there are lighter libraries for this? Or we could do server side rendering for these calendars... I know we like CLNDR
On Tue, Jul 9, 2019 at 2:20 PM Stuart Romanek notifications@github.com wrote:
@stuartromanek commented on this pull request.
In lib/modules/apostrophe-events-calendar-widgets/lib/api.js https://github.com/apostrophecms/apostrophe-events/pull/40#discussion_r301728440 :
+
- var max = moment(month).add({ days: 15, months: 1 }).format('YYYY-MM-DD');
- var min = moment(month).subtract(15, 'd').format('YYYY-MM-DD');
- var query = { startDate: { $lte: max, $gte: min } }
- var projection = _.merge((self.options.projection || {}), { startDate:1, title: 1, url: 1, type:1, slug:1 });
- var events = await self.apos.modules[self.piecesModuleName].find(req, query, projection).toArray();
- events.forEach(event => {
- event.date = event.startDate
- });
- var clndrOptions = {
- template: self.render(req, '_clndr', {}),
- daysOfTheWeek: ['Su', 'M', 'T', 'W', 'Th', 'F', 'S'],
- events: events,
- moment: moment,
broke this out as its own option to push moment.js.
if apostrophe-assets is set to lean: true and your calendar module is NOT set to pushMoment: false, you get moment along side the other assets.
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-events/pull/40?email_source=notifications&email_token=AAAH27NVUE7QFWCA7B3THYTP6TJIFA5CNFSM4H4ANG2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB55GG4Q#discussion_r301728440, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAH27MDAFPVQQQETHFWDNLP6TJIFANCNFSM4H4ANG2A .
--
Thomas Boutell, Chief Software Architect P'unk Avenue | (215) 755-1330 | punkave.com
Hang on, this isn't lean compatible at all, it depends on jquery.
We should just not suggest it has any lean support, and rely on the fact that moment does get pushed "always" when lean is not turned on.
If you need this in a lean project it needs a rethink - no jquery and no moment, I'd say, or if moment is truly necessary it should be imported in a webpack asset build so it's not in the global namespace after you add this module.
Closing for inactivity. Not deleting, though.
Provides a boilerplate calendar widget that can be put on any page.
Can be paged back and forth with arrows.
Each back/forward motion requests the next/previous month (and a little bit extra, if you wanted to style the ghosted days of the next/prev month that appear on the calendar). No grab-all-the-stuff.
No filtering of content as of now .. could be built in in the future.
Once we get this baseline part of the bundle published I'll get a demo of it in Open Museum.